This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Replaces TEST_IS_CONSTANT_EVALUATED.
AbandonedPublic

Authored by Mordante on Jun 16 2022, 8:58 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Summary

When using GCC-12 in C++11 mode the TEST_IS_CONSTANT_EVALUATED macro
causes "tautological compare" errors. This replaces the macro with a
helper function. The function is based on @philnik's suggestion in
D126667.

Diff Detail

Event Timeline

Mordante created this revision.Jun 16 2022, 8:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 8:58 AM
Mordante updated this revision to Diff 437588.Jun 16 2022, 10:11 AM

Fixes C++03 build.

Mordante updated this revision to Diff 437910.Jun 17 2022, 8:10 AM

Fixes CI.

Mordante published this revision for review.Jun 17 2022, 11:57 AM
Mordante added reviewers: ldionne, philnik.

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.

Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 11:57 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik accepted this revision as: philnik.Jun 17 2022, 12:07 PM

LGTM % nits.

libcxx/test/support/constant_evaluated.h
18

Can't this be an anonymous enum? Or is this an extension?

27–31

I think I'd rather have this in it's own function instead of a macro.

Mordante marked an inline comment as done.Jun 17 2022, 12:23 PM
Mordante added inline comments.
libcxx/test/support/constant_evaluated.h
18

No it can't; it's used as an argument to is_constant_evaluated.

27–31

But that function would also need macros, right?
I can add a function that only returns the status of "IS_CONSTANT_EVALUATED" and keep the version dependent logic here.

jloser added a subscriber: jloser.Jun 18 2022, 5:21 AM
jloser added inline comments.
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?

Mordante marked 2 inline comments as done.Jun 20 2022, 9:04 AM
Mordante added inline comments.
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.

ldionne requested changes to this revision.Jun 20 2022, 12:25 PM

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.

This revision now requires changes to proceed.Jun 20 2022, 12:25 PM
ldionne added inline comments.Jun 20 2022, 12:34 PM
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.

Mordante abandoned this revision.Aug 31 2023, 11:25 AM
Mordante marked an inline comment as done.

Based on the review comments, I'm no longer pursuing this patch.