Page MenuHomePhabricator

[clang-tidy] Fix RenamerClangTidy checks breaking lambda captures.
Needs ReviewPublic

Authored by njames93 on Feb 28 2021, 6:35 PM.

Details

Summary

Currently these checks will overwrite the default capture token (=|&) if any default captures refer to decls that need fixing.
There was an a previous patch (D59540) that tried to address this shortfall but it went about it trying to detect if it was inside a lambda then comparing the textual representation.
Learning from the previous its obvious that we need someway to tag a DeclRefExpr.
I've settled on RefersToDefaultCapture for now, but there could be a case to make it IsImplicit, for when a DeclRefExpr isn't actually written.
If anyone can think of good examples when this could happen I'd definitely support that instead.
Right now I've only addressed lambdas as I don't know anything about ObjectiveC Blocks, If this makes sense there I'd be happy to extend it.

Diff Detail

Unit TestsFailed

TimeTest
470 msx64 debian > MemProfiler-x86_64-linux.TestCases::test_malloc_load_store.c
Script: -- : 'RUN: at line 5'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/memprof/TestCases/test_malloc_load_store.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/memprof/X86_64LinuxConfig/TestCases/Output/test_malloc_load_store.c.tmp

Event Timeline

njames93 created this revision.Feb 28 2021, 6:35 PM
njames93 requested review of this revision.Feb 28 2021, 6:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2021, 6:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Where would be a good place for testing refersToDefaultCapture support?

clang/lib/Sema/SemaLambda.cpp
1619

Correct me if I'm wrong, but there shouldn't be a case where the assert in cast fails.

clang/lib/Serialization/ASTWriterDecl.cpp
2285–2286

I have no idea about BitcodeWriter, is this correct or should I just be using the literal abbreviation?

njames93 updated this revision to Diff 327019.Feb 28 2021, 7:01 PM

Add a test case using init capture.

martong removed a subscriber: martong.Mar 8 2021, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2021, 7:08 AM

Where would be a good place for testing refersToDefaultCapture support?

Thank you for your patience while I thought about this a bit more, sorry for the delay in reviewing it!

I think it probably makes sense to add testing for it in clang\test\AST with an AST dumping test (so the text node dumper would probably need to be updated to mention that the DeclRefExpr is an implicit capture).

clang/lib/Sema/SemaLambda.cpp
1619

Hmmm, could it be an unresolved lookup expr in the case of a dependent capture? Or a MemberExpr in the case of capturing an anonymous structure or union member capture?

Because we're trying to determine if a DeclRefExpr was implicitly captured, would it be better to use "implicit capture" in the identifier rather than "default capture"?

Where would be a good place for testing refersToDefaultCapture support?

Thank you for your patience while I thought about this a bit more, sorry for the delay in reviewing it!

I think it probably makes sense to add testing for it in clang\test\AST with an AST dumping test (so the text node dumper would probably need to be updated to mention that the DeclRefExpr is an implicit capture).

That sounds like a good place to start, thanks.

clang/lib/Sema/SemaLambda.cpp
1619

Can't capture anonymous members so thats a non issue. I'm not entirely sure about dependent, but the codepaths where isVariableCapture is true always seem to result in either an ExprError or DeclRefExpr.

I do agree with the name though.

njames93 updated this revision to Diff 336645.Apr 10 2021, 11:44 PM

Use implicit capture instead of default capture.

@aaron.ballman, Unfortunately the AST dump tests don't work as implicit captures don't appear to show up in the textual representation.

Use implicit capture instead of default capture.

@aaron.ballman, Unfortunately the AST dump tests don't work as implicit captures don't appear to show up in the textual representation.

Yeah, I was suggesting to add that as part of this patch so it'd be testable. We currently print something like -DeclRefExpr <col:14> 'int' lvalue Var 0x55b0ddc7f808 'b' 'int' and I think if we changed TextNodeDumper::VisitDeclRefExpr() to also print out RefersToEnclosingVariableOrCapture and RefersToImplicitCapture only when they're set, we could output something like: -DeclRefExpr <col:14> 'int' lvalue Var 0x55b0ddc7f808 'b' 'int' implicit_capture or -DeclRefExpr <col:14> 'int' lvalue Var 0x55b0ddc7f808 'b' 'int' explicit_capture, which would be useful (in addition to making your changes testable). WDYT?