Page MenuHomePhabricator

Adding 4 ASTMatchers: typedefDecl, isInMainFile, isInSystemFile, isInFileMatchingName
ClosedPublic

Authored by parallaxe on Jun 24 2014, 4:08 PM.

Details

Reviewers
klimek
Group Reviewers
deleted
Summary

I'm often only interested in matches within the main-file or matches that are not within a system-header, so for this purpose i added the two matchers isInMainFile and isInSystemFile. They take no arguments and narrow down the matches. The isInFileMatchingName is mainly thought for interactive clang-query-sessions, to make a matcher more specific without restarting the session with the files you are interested in for that moment. It takes a string that will be used as regular-expression to match the filename of where the matched node is expanded.

I was just curious that there is no typedefDecl-matcher, so I'm adding this one to this commit, too.

I'm not sure if the tests are sufficient but I have no good clue what tests would add more value.

Diff Detail

Event Timeline

parallaxe updated this revision to Diff 10814.Jun 24 2014, 4:08 PM
parallaxe retitled this revision from to Adding 4 ASTMatchers: typedefDecl, isInMainFile, isInSystemFile, isInFileMatchingName.
parallaxe updated this object.
parallaxe edited the test plan for this revision. (Show Details)
parallaxe changed the visibility from "Public (No Login Required)" to "parallaxe (Hendrik von Prince)".
parallaxe changed the edit policy from "All Users" to "parallaxe (Hendrik von Prince)".
parallaxe added a project: deleted.
parallaxe changed the visibility from "parallaxe (Hendrik von Prince)" to "Public (No Login Required)".
parallaxe changed the edit policy from "parallaxe (Hendrik von Prince)" to "Administrators".
parallaxe added a subscriber: Unknown Object (MLST).
parallaxe changed the edit policy from "Administrators" to "All Users".Jun 25 2014, 3:40 PM
sbenza added a subscriber: sbenza.Jul 8 2014, 9:52 AM
sbenza added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
155

s/typedefType/typedefDecl/

205

Example is copy-pasted from above and not updated for isInFileMatchingName

include/clang/Tooling/Tooling.h
157

I believe we can't still use brace init in clang. More instances of this in other files.
See http://llvm.org/docs/CodingStandards.html#supported-c-11-language-and-library-features

lib/ASTMatchers/Dynamic/Registry.cpp
315

sort lines.

unittests/ASTMatchers/ASTMatchersTest.cpp
4406

Use matches() or notMatches() instead.

4440

Comment why this dies.

4441

You don't need the whole EXPECT_TRUE expression here.
Just the matcher is enough.

unittests/ASTMatchers/ASTMatchersTest.h
65

over 80 chars. reflow the signature.

djasper added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
155

so, this should be typedefDecl, not typedefType. Also, remove the "isInMainFile()" as that is confusing.

202

Please indicate whether this is a partial match or a full match.

205

Copy and paste error.

219

I think it is unnecessary, to assert here. The behavior is well-defined.

include/clang/Tooling/Tooling.h
157

Stick to 80 columns.

unittests/ASTMatchers/ASTMatchersTest.cpp
4440

This test should go away with the assert.

unittests/ASTMatchers/ASTMatchersTest.h
65

Stick to 80 columns.

klimek edited edge metadata.Sep 8 2014, 3:04 AM

Is this still ongoing?

alexfh removed a reviewer: alexfh.Oct 13 2014, 6:01 AM
In D4283#15, @klimek wrote:

Is this still ongoing?

Yes! I'm sorry i didn't react on this for so long, i am currently updating the patch according to the reviews (thanks by the way!)

parallaxe updated this revision to Diff 15123.Oct 18 2014, 2:06 PM

I updated it accordingly to the comments (thanks!).
One of the changes is the replacement of an initializer list through an explicit call to the default-constructor of std::vector<std::pair<std::string, std::string>>, to initialize the new introduced VirtualMappedFiles-parameter of the functions runToolOnCodeWithArgs and matchesConditionally. Due to the repeated usage of the type-declaration, it would be nice to declare a typedef for std::vector<std::pair<std::string, std::string>>, that could also be used for the RemappedFiles-instance-variable of the class PreprocessorOptions.
If it is appropriate to add the typedef, where should I add it and how should I name it?

Thanks a lot for working on this - we're getting close :) Sorry for the delay in the review.

include/clang/ASTMatchers/ASTMatchers.h
169

Perhaps call it isExpansionInMainFile? Because we might want isSpellingInMainFile at some point. Same below.

215

I'd again add Expansion and then leave the name out: isExpansionInFileMatching("...") seems to read nicely ...

include/clang/Tooling/Tooling.h
155

Yea, typedef seems in order - perhaps FileMappings? OverlayMappings? OverlayedFileMappings? OverlayedFileContent? FileContentMappings? just throwing a few ideas out. The right spot is to put it into this header, perhaps directly above where it's first used (which for now is probably here).

parallaxe updated this revision to Diff 15460.Oct 26 2014, 10:36 AM

isExpansionInMainFile and isExpansionInFileMatching - sounds reasonable :)
I'm going with FileContentMappings for now, but feel free to change anytime - I'm not enthusiastic about names.
I included Tooling.h in PreprocessorOptions.h to have access to the new typedef FileContentMappings. It feels kinda heavy to add that include for one typedef only. Furthermore, I'm unsure how to update the description of the RemappedFiles, as it directly relates to the std::pair that is now no longer visible in the typename. In my eyes, the usage of std::pair now becomes to an implementation-detail that should not be exposed in the comment, so i removed these mentions.

klimek added inline comments.Oct 30 2014, 9:58 AM
include/clang/ASTMatchers/ASTMatchers.h
156

Nit: "." at the end of sentences (here and below).

190

Also rename to isExpansionInSystemHeader?

include/clang/Lex/PreprocessorOptions.h
14

We don't want the Lexer to depend on Tooling - that's the wrong way around...

99

I'd just leave this without the typedef for now...

parallaxe updated this revision to Diff 15653.Oct 31 2014, 3:02 PM
  • Reverted the changes in PreprocessorOptions.h
  • Updated the descriptions of the added matchers (added the dots and updated the matcher-names in their descriptions, which i forgot before)
  • Renamed isInSystemHeader to isExpansionInSystemHeader
klimek accepted this revision.Oct 31 2014, 4:04 PM
klimek edited edge metadata.

lg. Thanks!

This revision is now accepted and ready to land.Oct 31 2014, 4:04 PM

Glad that my patch got accepted, thanks!
Well, as I have no commit access I thought that someone might apply that patch by himself - or that there is a magic mechanism that would have allowed me to commit the patch by using arcanist. Apparently, both assumptions seem to be wrong - may somebody apply that patch to the tree?

include/clang/ASTMatchers/ASTMatchers.h
219

I took that from the existing matchesName-matcher (Line 1707 currently). Is there a difference between the matchesName-matcher and the isInFileMatchingName-matcher that makes sense to have a different behaviour regarding of empty regex? Or should that assertion also be removed from the matchesName-regex?

include/clang/Tooling/Tooling.h
157

I see. So replacing the empty initializer list by const std::vector<std::pair<std::string, std::string>>() would do the thing, but repeating the nesting template definition doesn't appeal me. A typedef could improve it.
Searching for std::vector<std::pair<std::string, std::string>>in the project brings the RemappedFile-variable in PreprocessorOptions.h up, so it would make sense to declare the type at a level where it could be also reused in PreprocessorOptions.h, wouldn't it?
I would be lucky of some advices to this:

  • should I introduce a typedef for that?
  • where would be a good place?
  • what should it be named? (my first guess would be something like "FileMappings", but I'm honestly not so good in choosing names)

I'll happily land the CL once llvm.org is up again

include/clang/ASTMatchers/ASTMatchers.h
219

You also seem to have resolved this (same problem as other comment)?

Also, it's generally a good idea to add a reply with a short comment "Done." to all comments people have made so it's clear they have been addressed.

include/clang/Tooling/Tooling.h
157

I assume this was something you didn't send off before? (Note that you have to click "Submit" after adding new comments, otherwise they will not be visible, even when you upload a new patch with arc)

So, what's the status? Anything wrong with it / anything I can do?

This was rolled back due to windows breakage.

Re-applied in r222765. Let's see whether it sticks this time...

Oh, great! Thanks for adapting it that it will (hopefully) don't cause any more problems on windows!

klimek closed this revision.Jul 3 2015, 7:03 AM