Header files included wrongly using <...> are now included using the
internal path names as the new unittest framework allows us to do so.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/src/sys/mman/munmap.h | ||
---|---|---|
12 | While we are here, the other comments end with a period so this one should too. | |
libc/test/src/string/strcat_test.cpp | ||
21 | Shouldn't we keep this as just ASSERT_EQ, the streq doesn't make much sense because these should be the same pointers. I think right now ASSERT_EQ with char* will not work but see my other comments. | |
libc/utils/UnitTest/Test.cpp | ||
43 | Maybe a using llvm_libc::testing::Test; and shorten these to Test::Cond_EQ? | |
48 | What about llvm::withColor(llvm::errs(), llvm::raw_ostream::RED); Even if you don't like the color, all of these should be printing to stderr. | |
133 | What if addTest added to a vector that way we can easily change the order the tests are executed for like in --gtest_shuffle | |
151 | for (Test *T = Start; T; T = T->Next, ++TestCount) And then the Test *T = Start; if (T == nullptr) return 0; can be removed along with the ++TestCount right below this comment. | |
175 | What about !!FailCount? | |
178–184 | You can create template <typename T> bool Test::test(...) { return ::test(...); } and then instantiate each of these template Test::test<int>(RunContext &, ...); But see previous comment I think we can do this differently. | |
libc/utils/UnitTest/Test.h | ||
56–59 | We can make this template call ::test(...) just here. But also It would be worth it to have enable_if specialization for std::is_pointer to call ::test<const void*>, and std::is_integral to call ::test<int64_t> and one for unsigned integrals to call use uint64_t so that we only need to have a couple instantiated in Test.cpp. | |
103–106 | What do you think about making the ASSERT_*'s into if (!EXPECT_*) return? |
Address comments.
libc/test/src/string/strcat_test.cpp | ||
---|---|---|
21 | I have added a new ASSERT_PTREQ macro to test pointer equality. I previously had a specialization of ASSERT_EQ for void *. But, I think something like a PTREQ it is more useful as it can be used to compare any kind of pointer. | |
libc/utils/UnitTest/Test.cpp | ||
43 | Its affecting only this function so I have left it as is. | |
48 | Streaming to llvm::errs() instead of llvm::outs(). | |
133 | Yeah, I did consider that. But, that means we will have a global non-POD which is usually not considered as a good practice. We can always reconsider this in case we want features like --gtest-shuffle. | |
175 | Personally, I do not like the implicit nature of such a construct. | |
178–184 | I do not think a declaration of a function specialization instantiates it. Am I misunderstanding something? | |
libc/utils/UnitTest/Test.h | ||
56–59 | This a very good suggestion to enable_if. But, I do not want to include <type_traits> here so I have now implemented an equivalent and used it to incorporate your suggestion. The benefit of using it is that the use of unsupported types leads to a compile-error which is more descriptive than a link-error (which was the case earlier.) About calling ::test, since ::test is template function, we will have to define it in the header. We will then have to include some headers which we want to avoid. | |
103–106 | This is yet another good suggestion. Done now. |
libc/utils/UnitTest/Test.cpp | ||
---|---|---|
15 | This include is unnecessary I think. | |
149 | This one is still to outs | |
156 | ditto | |
159 | ditto | |
165 | ditto | |
178–184 | It instantiates if you write like template function<params_to_instantiate>(...);. Its used more often with class templates but it works with function templates as well. In llvm, I have seen it here. | |
libc/utils/UnitTest/Test.h | ||
24 | It looks like libcxx does remove_cv before these specializations. Which confuses me because template <typename T> void f(T) {} template <> void f<char>(char) {} int main() { const volatile char c = 'c'; f(c); // calls f<char> } But I figured I would mention it. | |
56–59 |
Thats no different than what we have now we have the declaration of Test::test and it's definition in the cpp file. Just putting template <typename ValType> bool ::test(...); in the header file and then making Test::test call ::test makes it inline-able but more importantly its a little easier to follow I think.
I think it is important that we make a IsPointerType specialization so we can just use EXPECT_EQ and be consistent with gtest. This will confuse people later I'm pretty sure. Also if we do the above suggestion this will be easy to make this specialization call ::test<unsigned long long>. |
I just checked quickly and it looks like gtest actually prints to stdout which surprised me, I always figured it was to stderr. Do you think we should go back to stdout?
TBF, I do not have any opinion here. For me, a message from a test run is all useful. So, its hard for me to see why something should be logged to stderr and something else to stdout. I have now put everything back to stdout because I actually think there could be tools out there which are probably only looking at stdout.
libc/utils/UnitTest/Test.cpp | ||
---|---|---|
149 | The others were really an error message, so may be stderr was more appropriate. These are purely informational and hence I left them with stdout. | |
178–184 | Ah, I never used this before. I have modified as you have suggested now. | |
libc/utils/UnitTest/Test.h | ||
56–59 | I don't have any opinion about this. I had previously thought I should limit the re-implementation of std types. But heck, we have already come this far so I have implemented everything as you have suggested. |
I modified to use switch statements as suggested before submitting.
libc/test/config/linux/x86_64/syscall_test.cpp | ||
---|---|---|
12 | Yes, it should go. I have kept it for now as I have a different plan for things like these. Will write about them in detail soon. | |
libc/test/src/string/strcat_test.cpp | ||
35–36 | No harm to keep them I guess :) | |
libc/utils/UnitTest/Test.h | ||
96 | The main function can then just return the result of runTests. I don't think bool return value disallows it, but I don't like the implicit nature of such conversions. |
While we are here, the other comments end with a period so this one should too.