Page MenuHomePhabricator

Ignore FullExpr when traversing cast sub-expressions
ClosedPublic

Authored by kimgr on Feb 10 2022, 1:13 PM.

Details

Reviewers
aaron.ballman
Summary

Full-expressions are Sema-generated implicit nodes that cover
constant-expressions and expressions-with-cleanup for temporaries.

Ignore those as part of implicit-ignore, and also remove too-aggressive
IgnoreImplicit (which includes nested ImplicitCastExprs, for example) on
unpacked sub-expressions.

Add some unittests to demonstrate that RecursiveASTVisitor sees through
ConstantExpr nodes correctly.

Adjust cxx2a-consteval test to cover diagnostics for nested consteval
expressions that were previously missed.

Fixes bug #53044.

Depends on D119476

Diff Detail

Unit TestsFailed

TimeTest
510 msx64 debian > XRay-x86_64-linux.TestCases/Posix::basic-filtering.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fxray-instrument -m64 -std=c++11 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/xray/TestCases/Posix/basic-filtering.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/xray/X86_64LinuxConfig/TestCases/Posix/Output/basic-filtering.cpp.tmp -g
2,260 msx64 debian > libFuzzer.libFuzzer::reduce_inputs.test
Script: -- : 'RUN: at line 3'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/reduce_inputs.test.tmp/C

Event Timeline

kimgr requested review of this revision.Feb 10 2022, 1:13 PM
kimgr created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2022, 1:13 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@aaron.ballman This patch replaces https://reviews.llvm.org/D117391 as a potential solution for https://github.com/llvm/llvm-project/issues/53044.

I can't swear that the new diagnostics in cxx2a-consteval.cpp are standards-conforming, but I think they are no less correct than what's already there. Fingers crossed :-)

I haven't had the chance to review this yet (in C standards meetings again this week), but I did notice that https://reviews.llvm.org/D119095 seems to be touching on a related (or possibly the same) issue. Can you coordinate with the other patch author to make sure nobody is writing patches (or reviewing them) that conflict? Thanks!

kimgr added a comment.Feb 27 2022, 4:18 AM

I have a local branch with both D119476 and this patch.

  • Rebased on latest main (853ca5472314e109b98e46f0985f27f79e17d2bd)
  • Ran ninja check-clang and ninja tools/clang/unittests/Tooling/ToolingTests && tools/clang/unittests/Tooling/ToolingTests without issue

I did some integration testing with D119095 and could not find any conflict. More detail on that differential.

kimgr added a comment.Mar 6 2022, 10:55 PM

Gentle weekly ping

Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2022, 10:55 PM
kimgr added a comment.Mar 14 2022, 1:30 PM

@aaron.ballman Friendly Monday ping.

The changes here generally look good, and I'm happy to see that more of our test cases are diagnosing similar to other compilers with the changes.

clang/test/SemaCXX/cxx2a-consteval.cpp
365

Why are we dropping this test coverage?

371–372

I usually prefer line continuation characters because I think it makes the test easier to read (it's easy to miss secondary diagnostics on the same line). However, I don't insist on these changes either (but if you make them, please do similar for the other test lines you're touching).

kimgr added inline comments.Mar 18 2022, 9:32 AM
clang/test/SemaCXX/cxx2a-consteval.cpp
365

Good question, that must've been a mistake. I'll take another look.

371–372

Thanks, I wasn't aware there was support for line continuation. I agree it would benefit readability here, so I'll look into it.

kimgr updated this revision to Diff 416695.Mar 19 2022, 7:13 AM

Address review feedback

  • Restore accidentally removed test line
  • Clarify diagnostic expectations
aaron.ballman accepted this revision.Mar 21 2022, 12:05 PM

The changes here LGTM!

This revision is now accepted and ready to land.Mar 21 2022, 12:05 PM

I've commit on your behalf in 403d7d8d7093d6637a006f8b3f75382294259d3f, and I'm going to add a release note in a separate commit as I missed that feedback during the review.

I've commit on your behalf in 403d7d8d7093d6637a006f8b3f75382294259d3f, and I'm going to add a release note in a separate commit as I missed that feedback during the review.

6b2335cace8378f499b401bcda3f6a862812539a contains the release note.