[libcxx-commits] [PATCH] D127978: [libc++][test] Replaces TEST_IS_CONSTANT_EVALUATED.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp:58
 
-            if (!TEST_IS_CONSTANT_EVALUATED || TEST_STD_VER >= 20) {
+            if (!is_constant_evaluated(TestedCppVersion::Cpp14) || TEST_STD_VER >= 20) {
                 assert((std::basic_string<CharT, Traits>(v[i]) == v[j]) == expected);
----------------
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.


================
Comment at: libcxx/test/std/strings/string.view/string.view.comparison/equal.pass.cpp:79
 
-    if (!TEST_IS_CONSTANT_EVALUATED || TEST_STD_VER >= 20) {
+    if (!is_constant_evaluated(TestedCppVersion::Cpp14) || TEST_STD_VER >= 20) {
         assert((std::basic_string<CharT, Traits>(abc) == abc0def) == false);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127978/new/

https://reviews.llvm.org/D127978