Skip to content

Commit 1c5c8de

Browse files
msluszniakMateusz Sluszniak
andauthored
Setup oss to print bools correctly out-of-the-box & add three tests (#444)
## Description As in the title, tiny changes in handling boolean values and added two tests. ### Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation update (improves or adds clarity to existing documentation) ### Tested on - [x] iOS - [ ] Android ### Testing instructions Run test with provided scripts and check if they pass ### Screenshots <!-- Add screenshots here, if applicable --> ### Related issues <!-- Link related issues here using #issue-number --> ### Checklist - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings ### Additional notes <!-- Include any additional information, assumptions, or context that reviewers might need to understand this PR. --> --------- Co-authored-by: Mateusz Sluszniak <sluszmat@amazon.com>
1 parent 3f939cc commit 1c5c8de

2 files changed

Lines changed: 99 additions & 30 deletions

File tree

packages/react-native-executorch/common/rnexecutorch/Log.h

Lines changed: 69 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
#include <cstdint>
33
#include <exception>
44
#include <filesystem>
5+
#include <ios>
56
#include <iostream>
67
#include <iterator>
78
#include <memory>
@@ -50,13 +51,28 @@ concept HasPop = requires(T t) {
5051
};
5152

5253
template <typename T>
53-
concept HasEmpty = requires(T t) {
54-
{ t.empty() } -> std::convertible_to<int>;
54+
concept HasEmpty = requires(const T &t) {
55+
{ t.empty() } -> std::same_as<bool>;
5556
};
5657

58+
// These two below are needed apart from TopAccessible and FrontAccessible
59+
// to guarantee that correct templated is matched
5760
template <typename T>
58-
concept Sequencable =
59-
HasPop<T> && HasEmpty<T> && (FrontAccessible<T> || TopAccessible<T>);
61+
concept ReadOnlySequencableFront = requires(const T &t) {
62+
{ t.front() } -> std::same_as<const typename T::value_type &>;
63+
} && HasEmpty<T> && !Iterable<T>;
64+
65+
template <typename T>
66+
concept ReadOnlySequencableTop = requires(const T &t) {
67+
{ t.top() } -> std::same_as<const typename T::value_type &>;
68+
} && HasEmpty<T> && !Iterable<T>;
69+
70+
template <typename T>
71+
concept ReadOnlySequencable =
72+
ReadOnlySequencableFront<T> || ReadOnlySequencableTop<T>;
73+
74+
template <typename T>
75+
concept MutableSequencable = ReadOnlySequencable<T> && HasPop<T>;
6076

6177
template <typename T>
6278
concept Streamable = requires(std::ostream &os, const T &t) {
@@ -79,13 +95,12 @@ concept WeakPointer = requires(const T &a) {
7995
};
8096

8197
template <typename T>
82-
concept Fallback = !Iterable<T> && !Sequencable<T> && !Streamable<T> &&
83-
!SmartPointer<T> && !WeakPointer<T>;
98+
concept Fallback =
99+
!Iterable<T> && !Streamable<T> && !SmartPointer<T> && !WeakPointer<T> &&
100+
!ReadOnlySequencable<T> && !MutableSequencable<T>;
84101

85102
} // namespace concepts
86103

87-
void printElement(std::ostream &os, bool value);
88-
89104
template <typename T>
90105
requires concepts::Streamable<T> && (!concepts::SmartPointer<T>)
91106
void printElement(std::ostream &os, const T &value);
@@ -103,9 +118,15 @@ template <typename T>
103118
requires concepts::Iterable<T> && (!concepts::Streamable<T>)
104119
void printElement(std::ostream &os, const T &container);
105120

121+
template <typename T> void printSequencable(std::ostream &os, T &&container);
122+
106123
template <typename T>
107-
requires concepts::Sequencable<T>
108-
void printElement(std::ostream &os, T container);
124+
requires concepts::ReadOnlySequencable<T>
125+
void printElement(std::ostream &os, const T &container);
126+
127+
template <typename T>
128+
requires concepts::MutableSequencable<T>
129+
void printElement(std::ostream &os, T &&container);
109130

110131
template <typename... Args>
111132
void printElement(std::ostream &os, const std::tuple<Args...> &tpl);
@@ -132,10 +153,6 @@ void printElement(std::ostream &os,
132153
template <concepts::Fallback UnsupportedArg>
133154
void printElement(std::ostream &os, const UnsupportedArg &value);
134155

135-
void printElement(std::ostream &os, bool value) {
136-
os << (value ? "true" : "false");
137-
}
138-
139156
void printElement(std::ostream &os, const std::error_code &ec);
140157

141158
template <typename T>
@@ -189,30 +206,46 @@ void printElement(std::ostream &os, const T &container) {
189206
os << "]";
190207
}
191208

192-
template <typename T>
193-
requires concepts::Sequencable<T>
194-
void printElement(
195-
std::ostream &os,
196-
T container) { // Pass by value to prevent modifications to the original
209+
template <typename T> void printSequencable(std::ostream &os, T &&container) {
197210
os << "[";
198-
199211
bool isFirst = true;
200-
while (!container.empty()) {
212+
213+
auto printElementLambda = [&isFirst, &os](auto &&element) {
201214
if (!isFirst) {
202215
os << ", ";
203216
}
217+
low_level_log_implementation::printElement(
218+
os, std::forward<decltype(element)>(element));
219+
isFirst = false;
220+
};
221+
222+
while (!container.empty()) {
204223
if constexpr (concepts::FrontAccessible<T>) {
205-
printElement(os, container.front());
224+
printElementLambda(container.front());
206225
} else if constexpr (concepts::TopAccessible<T>) {
207-
printElement(os, container.top());
226+
printElementLambda(container.top());
208227
}
209228
container.pop();
210-
isFirst = false;
211229
}
212230

213231
os << "]";
214232
}
215233

234+
template <typename T>
235+
requires concepts::ReadOnlySequencable<T>
236+
void printElement(std::ostream &os, const T &container) {
237+
T tempContainer = container; // Make a copy to preserve original container
238+
printSequencable(
239+
os, std::move(tempContainer)); // Use std::move since tempContainer won't
240+
// be used again
241+
}
242+
243+
template <typename T>
244+
requires concepts::MutableSequencable<T>
245+
void printElement(std::ostream &os, T &&container) {
246+
printSequencable(os, std::forward<T>(container));
247+
}
248+
216249
template <typename... Args>
217250
void printElement(std::ostream &os, const std::tuple<Args...> &tpl) {
218251
os << "<";
@@ -388,6 +421,12 @@ std::string getBuffer(const std::string &logMessage,
388421
return logMessage;
389422
}
390423

424+
std::ostringstream createConfiguredOutputStream() {
425+
std::ostringstream oss;
426+
oss << std::boolalpha;
427+
return oss;
428+
}
429+
391430
} // namespace high_level_log_implementation
392431

393432
/**
@@ -416,14 +455,15 @@ std::string getBuffer(const std::string &logMessage,
416455
* Nothing.
417456
*/
418457
template <std::size_t MaxLogSize = 1024, typename... Args>
419-
void log(LOG_LEVEL logLevel, const Args &...args) {
420-
std::ostringstream oss;
421-
auto space = [&oss](const auto &arg) {
422-
low_level_log_implementation::printElement(oss, arg);
458+
void log(LOG_LEVEL logLevel, Args &&...args) {
459+
auto oss = high_level_log_implementation::createConfiguredOutputStream();
460+
auto space = [&oss](auto &&arg) {
461+
low_level_log_implementation::printElement(
462+
oss, std::forward<decltype(arg)>(arg));
423463
oss << ' ';
424464
};
425465

426-
(..., space(args));
466+
(..., space(std::forward<Args>(args)));
427467

428468
// Remove the extra space after the last element
429469
std::string output = oss.str();

packages/react-native-executorch/common/rnexecutorch/tests/LogTest.cpp

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ namespace low_level_log_implementation {
2424

2525
class TestValue : public ::testing::Test {
2626
protected:
27+
TestValue() { oss << std::boolalpha; }
28+
2729
template <typename T>
2830
void testValueViaComparison(const T &value,
2931
const std::string &expectedOutput) {
@@ -487,12 +489,39 @@ TEST_F(LoggingTest, LoggingDoesNotChangeVector) {
487489
testLoggingDoesNotChangeContainer(original);
488490
}
489491

490-
TEST(LoggingTestTemplateArgument, LoggingWithNonDefaultLogSize) {
492+
TEST(LogFunctionTest, LoggingBasic) {
493+
EXPECT_NO_THROW(log(LOG_LEVEL::Debug, "Test123"));
494+
}
495+
496+
TEST(LogFunctionTest, LoggingWithNonDefaultLogSize) {
491497
constexpr std::size_t sizeBiggerThanDefault = 2048;
492498
const auto testString = std::string(sizeBiggerThanDefault, 'a');
493499
EXPECT_NO_THROW(log<sizeBiggerThanDefault>(LOG_LEVEL::Info, testString));
494500
}
495501

502+
TEST(LogFunctionTest, LoggingMoreThanOneElement) {
503+
constexpr auto testStringLiteral = "Test123";
504+
const auto testVector = std::vector<int>{1, 2, 3, 4};
505+
const auto testPair = std::pair<int, double>(1, 2.0);
506+
EXPECT_NO_THROW(
507+
log(LOG_LEVEL::Debug, testStringLiteral, testVector, testPair));
508+
}
509+
510+
TEST(MovingSequencable, MovingSequencableTest) {
511+
std::priority_queue<int> q;
512+
q.push(1);
513+
q.push(2);
514+
q.push(3);
515+
516+
log(LOG_LEVEL::Debug, q);
517+
ASSERT_EQ(q.size(), 3);
518+
const auto &cq = q;
519+
log(LOG_LEVEL::Debug, cq);
520+
ASSERT_EQ(cq.size(), 3);
521+
log(LOG_LEVEL::Debug, std::move(q));
522+
ASSERT_EQ(q.size(), 0);
523+
}
524+
496525
} // namespace rnexecutorch
497526

498527
int main(int argc, char **argv) {

0 commit comments

Comments
 (0)