Page MenuHomePhabricator

PR45881: Properly use CXXThisOverride for templated lambda
ClosedPublic

Authored by ychen on May 14 2021, 1:51 PM.

Details

Summary
  • this used in lambda expression parameter declarations needs no capture.
  • Set up CXXThisOverride for default template arguments of a lambda.

A similar fix to this is c3d2ebb60f604.

Diff Detail

Event Timeline

ychen requested review of this revision.May 14 2021, 1:51 PM
ychen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2021, 1:51 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Mostly just superficial comments from me at this point.

clang/lib/Sema/SemaTemplate.cpp
5114–5115
clang/lib/Sema/SemaTemplateDeduction.cpp
2865
2867
ychen updated this revision to Diff 346010.May 17 2021, 4:07 PM
  • Address comments.
tambre added a subscriber: tambre.Jul 5 2021, 11:53 PM
tambre added inline comments.Jul 5 2021, 11:56 PM
clang/test/SemaCXX/cxx2a-lambda-decltype-this.cpp
1–4 ↗(On Diff #346010)

-fblocks doesn't seem like it'd be necessary?

It would probably be a good idea to rename this to cxx20-lambda-decltype-this.cpp and use -std=c++20 from the get-go since C++20 has been finalized.

ychen updated this revision to Diff 356796.Jul 6 2021, 11:58 AM
  • address feedback
ychen marked 3 inline comments as done.Jul 6 2021, 11:58 AM
gulfem added a subscriber: gulfem.Wed, Sep 1, 10:43 AM

We recently enabled assertions in our Fuchsia builds, and our builds started to fail:

FAILED: obj/src/lib/storage/vfs/cpp/libcpp.fuchsia_vfs.cc.o
../../../recipe_cleanup/clangLhcV7J/bin/clang++ -MD -MF obj/src/lib/storage/vfs/cpp/libcpp.fuchsia_vfs.cc.o.d -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS=1 -…
clang++: clang/lib/Sema/SemaExprCXX.cpp:1144: clang::QualType adjustCVQualifiersForCXXThisWithinLambda(ArrayRef<clang::sema::FunctionScopeInfo *>, clang::QualType, clang::DeclContext *, clang::ASTC…
clang++: error: clang frontend command failed with exit code 134 (use -v to see invocation)

https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.core.x64-debug/b8837288282633988801/overview

This patch seems to be fixing the issue that we are running.
Is there a way to speed up the process in this review, so our broken builds can be fixed?

@aaron.ballman would it be possible for you to review this patch, so we can fix our broken builds?

aaron.ballman accepted this revision.Thu, Sep 2, 4:40 AM

LGTM, but please wait a day or so before landing in case @rsmith spots something I've missed (the only change I wasn't 100% certain on was lifting the assert predicate; it seems correct to me, but I'm wary of removing asserts).

This revision is now accepted and ready to land.Thu, Sep 2, 4:40 AM

Any update on this review? Our builds are still broken.

ychen added a comment.Tue, Sep 7, 10:50 AM

Any update on this review? Our builds are still broken.

I'll commit this by the end of the day if @rsmith wasn't able to take a look.

I'll commit this by the end of the day if @rsmith wasn't able to take a look.

Sounds good, thank you very much!

This revision was automatically updated to reflect the committed changes.