Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I've rebased the GCC-12 CI changes (D126667) on top of this one and the GCC CI is happy (https://buildkite.com/llvm-project/libcxx-ci/builds/11560).
So that means the only tautological comparison issues where the constant evaluated messages.
libcxx/test/std/strings/string.view/string.view.comparison/not_equal.pass.cpp | ||
---|---|---|
58 | I'll remove this comment before landing or the next upload whatever comes first. |
libcxx/test/std/strings/string.view/string.view.comparison/not_equal.pass.cpp | ||
---|---|---|
58 | That's interesting. Why does it fail in C++17 mode? |
libcxx/test/std/strings/string.view/string.view.comparison/not_equal.pass.cpp | ||
---|---|---|
58 | Initially I only used TestedCppVersion::Cpp20 which caused the build to file. After correcting this I forgot to remove this comment. |
While I have very little love for TEST_IS_CONSTANT_EVALUATED, I am not sure this increase in complexity for the reader is the right way to go forward in order to fix a (arguably kind of spurious) compiler warning. I'm eager to better understand the answer to the questions I've left and see if that makes me see this patch with different eyes.
libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp | ||
---|---|---|
58 | So the goal of this patch is to get rid of tautological comparison warnings in GCC. However, I am not sure this is a proper solution to this. Indeed, !is_constant_evaluated(TestedCppVersion::Cpp14) || TEST_STD_VER >= 20 could easily be flagged as tautological by GCC (or Clang) if for example TEST_STD_VER >= 20 evaluates to true. It looks like we've simply increased the complexity of the expression enough for GCC not to be as clever as ourselves, and hence it doesn't produce the warning. In other words, I am not convinced this is a long term or robust solution to this issue. | |
79 | I am not sure I understand what's the purpose of passing TestedCppVersion::CppNN to this function. Is there any case in which the NN version passed here should differ from the NN version used in the TEST_CONSTEXPR_CXXNN macro of the enclosing function? If not, then perhaps it doesn't really make sense to pass a standard version to this function? And if that's the case, then we're basically back to the TEST_IS_CONSTANT_EVALUATED macro again. |
libcxx/test/std/strings/basic.string/string.modifiers/string_assign/T_size_size.pass.cpp | ||
---|---|---|
22 | I think this is effectively what we want, unfortunately it makes the code pretty damn ugly because we would have to forward this bool across all function calls. |
I think this is effectively what we want, unfortunately it makes the code pretty damn ugly because we would have to forward this bool across all function calls.