Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 40622 Build 40745: arc lint + arc unit
Event Timeline
libcxx/include/__string | ||
---|---|---|
259 | I think it'd be better to have if (__libcpp_is_constant_evaluated()) { ... } else { ... } than just the early return. I normally don't care about this stuff, however in the case of is_constant_evaluated, I feel like having a clear if-else makes it clearer that we're doing the same thing in both branches, one in the compile-time world and one in the runtime world. Also, it might be a good idea to define a __memmove_constexpr function that we can call as-is from the else branch? |
This is looking good. I especially like how the tests require only minimal change between the runtime and the compile-time version. I think this is what we should strive for pretty much every time we constexpr-ify something going forward.
libcxx/include/__string | ||
---|---|---|
200 | This would be clearer (or something along those lines): // constexpr version of the C functions of the same name | |
203 | __memmove_constexpr? | |
216 | __memcpy_constexpr? | |
224 | __memset_constexpr? | |
657 | Is that necessary? | |
libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/assign3.pass.cpp | ||
20 | Did you forget to include the change to test_macros.h (or whatever file) where you add TEST_CONSTEXPR_CXX20? I can't find it. Also, I would suggest TEST_CONSTEXPR_AFTER_CXX17, to mirror the checks we usually make (which are always of the form #if STD > 17. |
libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/assign3.pass.cpp | ||
---|---|---|
20 |
Nevermind this, I just saw the other review. My bad.
Forget this, I'll comment on D68837 instead. |
Elaborated on the constexpr comment.
libcxx/include/__string | ||
---|---|---|
203 | I decided against this name here because this is more refactoring of the move algorithm for constexpr. Same line of reasoning for the below requests. | |
657 | Yes, this is necessary due to this test: assert(std::char_traits<char16_t>::move(NULL, s1, 0) == NULL); where, if n == 0, we should not perform the __s1 < __s2 check since that is UB. |
This would be clearer (or something along those lines):