Page MenuHomePhabricator

Add a matcher for members of an initializer list expression
ClosedPublic

Authored by hwright on Wed, Dec 26, 1:58 PM.

Details

Summary

Much like hasArg for various call expressions, this allows LibTooling users to match against a member of an initializer list.

This is currently being used as part of the abseil-duration-scale clang-tidy check.

Diff Detail

Repository
rL LLVM

Event Timeline

hwright created this revision.Wed, Dec 26, 1:58 PM
  1. Tests
  2. The docs file will need to be regenerated

@lebedev.ri Where do the appropriate tests live? (I couldn't find an obvious subdirectory in test/)
Where are the instructions for regenerating the documentation?

aaron.ballman requested changes to this revision.Mon, Dec 31, 7:12 AM

@lebedev.ri Where do the appropriate tests live? (I couldn't find an obvious subdirectory in test/)

This would live in clang\unittests\ASTMatchers\ASTMatchersNarrowingTest.cpp

Where are the instructions for regenerating the documentation?

Run clang\docs\tools\dump_ast_matchers.py and it will generate the documentation for you.

You should also update Registry.cpp to list the new matcher.

include/clang/ASTMatchers/ASTMatchers.h
3527 ↗(On Diff #179522)

I'm not certain we want the IgnoreParenImpCasts() here -- what if someone wants to match an initializer that uses one of those properties?

This revision now requires changes to proceed.Mon, Dec 31, 7:12 AM
hwright updated this revision to Diff 180089.Thu, Jan 3, 9:34 AM
hwright marked an inline comment as done.

Added tests, update docs.

include/clang/ASTMatchers/ASTMatchers.h
3527 ↗(On Diff #179522)

The hasArg implementation directly above has the same call to IgnoreParenImpCasts(). Is it also in error? (It would seem that they should both be consistent.)

aaron.ballman added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
3527 ↗(On Diff #179522)

Yeah, I noticed that as well. Doing some code archaeology, it seems that hasArg() has had that form since its inception and it was never explicitly discussed what should happen there.

@klimek -- do you have opinions here, since you wrote the original code for hasArg()? Should we be leaving the paren and implicit cast stripping up to the caller rather than doing it automatically?

Adding Manuel for the hasArg() questions.

klimek added inline comments.Fri, Jan 4, 2:13 AM
include/clang/ASTMatchers/ASTMatchers.h
3527 ↗(On Diff #179522)

Sigh, yea, this is one of those hard decisions: in the end, we decided that the downside of surprise of users not seeing what they expected (due to an imp cast) was less bad than basically taking away the ability to match imp casts, so we stopped putting ignoreparenimpcasts everywhere. I'm not entirely sure how to rate consistency with hasArg vs. getting us towards a more explicit world, but my gut would probably go with leaving the ignoreparenimpcasts out and see how bad people find it.

hwright updated this revision to Diff 180248.Fri, Jan 4, 9:00 AM

Removed IgnoreParenImpCasts call.

hwright marked 4 inline comments as done.Fri, Jan 4, 9:01 AM
hwright added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
3527 ↗(On Diff #179522)

Removed the call to IgnoreParenImpCasts().

aaron.ballman accepted this revision.Sat, Jan 5, 7:12 AM

LGTM! Thanks, both!

This revision is now accepted and ready to land.Sat, Jan 5, 7:12 AM
This revision was automatically updated to reflect the committed changes.