Page MenuHomePhabricator

[ASTMatchers] extract public matchers from const-analysis into own patch
ClosedPublic

Authored by JonasToth on Jan 10 2020, 6:51 AM.

Details

Summary

The analysis for const-ness of local variables required a view generally useful
matchers that are extracted into its own patch.

They are decompositionDecl and forEachArgumentWithParamType, that works
for calls through function pointers as well.

Diff Detail

Event Timeline

JonasToth created this revision.Jan 10 2020, 6:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 6:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Jan 10 2020, 7:52 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
4252–4253

if (const auto *Value = dyn_cast_or_null<ValueDecl>(Call->getCalleeDecl())) { ... }

4254

Should you be getting the canonical type here? That would skip over things like paren types and typedefs.

4259–4260

This seems like an invalid assertion -- K&R C functions would have a function pointer type but not a prototype, for instance.

4288

Should we be canonicalizing this type as well?

JonasToth marked 3 inline comments as done.Jan 10 2020, 8:13 AM
JonasToth added inline comments.
clang/include/clang/ASTMatchers/ASTMatchers.h
4288

I think here we shouldn't. In that case you could not match if e.g. the parameter has a typedef. I think that would reduce possibilities for the matcher usage.

JonasToth updated this revision to Diff 237338.Jan 10 2020, 8:14 AM
  • address review comments
aaron.ballman accepted this revision.Jan 10 2020, 8:37 AM

LGTM aside from a testing request.

clang/include/clang/ASTMatchers/ASTMatchers.h
4288

Okay, that's a good point!

clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
888

I would like to see one K&R C test case just to be sure we're not doing anything horrible:

void f();
void call_it_1(void) { f(1.0f, 2.0f); } // Especially curious about type promotions

void f(a, b) float a, b; { }
void call_it_2(void) { f(1.0f, 2.0f); } // And if they differ here (IIRC, we may have a bug here in the compiler)
This revision is now accepted and ready to land.Jan 10 2020, 8:37 AM
JonasToth updated this revision to Diff 237510.Jan 11 2020, 9:49 AM
JonasToth marked 3 inline comments as done.
  • add test for K&R functions
  • fix compilation error, missing getType
JonasToth marked an inline comment as done.Jan 11 2020, 10:03 AM
  • remove merge conflict markers in docs
Harbormaster completed remote builds in B43771: Diff 237514.
This revision was automatically updated to reflect the committed changes.