Adding 4 ASTMatchers: typedefDecl, isInMainFile, isInSystemFile, isInFileMatchingName

[parallaxe]

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.

[sbenza]

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]

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]

Is this still ongoing?

[parallaxe]

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]

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?

[klimek]

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]

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]

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 ↗

(On Diff #15460)

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

99 ↗

(On Diff #15460)

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

[parallaxe]

• 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]

lg. Thanks!

[parallaxe]

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)

[klimek]

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)

[parallaxe]

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

[klimek]

This was rolled back due to windows breakage.

[klimek]

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

[parallaxe]

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

include/clang/ASTMatchers/ASTMatchers.h

 
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchersInternal.h"
 #include "clang/ASTMatchers/ASTMatchersMacros.h"
 #include "llvm/ADT/Twine.h"

 /// Usable as: Any Matcher
 inline internal::TrueMatcher anything() { return internal::TrueMatcher(); }
 
+/// \brief Matches typedef declarations.
+///
+/// Given
+/// \code
+///   typedef int X;
+/// \endcode
+/// typedefDecl()
+///   matches "typedef int X"
+const internal::VariadicDynCastAllOfMatcher<Decl, TypedefDecl> typedefDecl;
+
+/// \brief Matches AST nodes that were expanded within the main-file.
+///
+/// Example matches X but not Y (matcher = recordDecl(isExpansionInMainFile())
+/// \code
+///   #include <Y.h>
+///   class X {};
+/// \endcode
+/// Y.h:
+/// \code
+///   class Y {};
+/// \endcode
+///
+/// Usable as: Matcher<Decl>, Matcher<Stmt>, Matcher<TypeLoc>
+AST_POLYMORPHIC_MATCHER(isExpansionInMainFile,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES_3(Decl, Stmt,
+                                                          TypeLoc)) {
+  auto &SourceManager = Finder->getASTContext().getSourceManager();
+  return SourceManager.isInMainFile(
+      SourceManager.getExpansionLoc(Node.getLocStart()));
+}
+
+/// \brief Matches AST nodes that were expanded within system-header-files.
+///
+/// Example matches Y but not X
+///     (matcher = recordDecl(isExpansionInSystemHeader())
+/// \code
+///   #include <SystemHeader.h>
+///   class X {};
+/// \endcode
+/// SystemHeader.h:
+/// \code
+///   class Y {};
+/// \endcode
+///
+/// Usable as: Matcher<Decl>, Matcher<Stmt>, Matcher<TypeLoc>
+AST_POLYMORPHIC_MATCHER(isExpansionInSystemHeader,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES_3(Decl, Stmt,
+                                                          TypeLoc)) {
+  auto &SourceManager = Finder->getASTContext().getSourceManager();
+  auto ExpansionLoc = SourceManager.getExpansionLoc(Node.getLocStart());
+  if (ExpansionLoc.isInvalid()) {
+    return false;
+  }
+  return SourceManager.isInSystemHeader(ExpansionLoc);
+}
+
+/// \brief Matches AST nodes that were expanded within files whose name is
+/// partially matching a given regex.
+///
+/// Example matches Y but not X
+///     (matcher = recordDecl(isExpansionInFileMatching("AST.*"))
+/// \code
+///   #include "ASTMatcher.h"
+///   class X {};
+/// \endcode
+/// ASTMatcher.h:
+/// \code
+///   class Y {};
+/// \endcode
+///
+/// Usable as: Matcher<Decl>, Matcher<Stmt>, Matcher<TypeLoc>
+AST_POLYMORPHIC_MATCHER_P(isExpansionInFileMatching,
+                          AST_POLYMORPHIC_SUPPORTED_TYPES_3(Decl, Stmt,
+                                                            TypeLoc),
+                          std::string, RegExp) {
+  auto &SourceManager = Finder->getASTContext().getSourceManager();
+  auto ExpansionLoc = SourceManager.getExpansionLoc(Node.getLocStart());
+  if (ExpansionLoc.isInvalid()) {
+    return false;
+  }
+  auto FileEntry =
+      SourceManager.getFileEntryForID(SourceManager.getFileID(ExpansionLoc));
+  if (!FileEntry) {
+    return false;
+  }
+
+  auto Filename = FileEntry->getName();
+  llvm::Regex RE(RegExp);
+  return RE.match(Filename);
+}
+
 /// \brief Matches declarations.
 ///
 /// Examples matches \c X, \c C, and the friend declaration inside \c C;

include/clang/Tooling/Tooling.h

 bool runToolOnCode(clang::FrontendAction *ToolAction, const Twine &Code,
                    const Twine &FileName = "input.cc");
 
+/// The first part of the pair is the filename, the second part the
+/// file-content.
+typedef std::vector<std::pair<std::string, std::string>> FileContentMappings;
+  
 /// \brief Runs (and deletes) the tool on 'Code' with the -fsyntax-only flag and
 ///        with additional other flags.
 ///

 /// \param FileName The file name which 'Code' will be mapped as.
 ///
 /// \return - True if 'ToolAction' was successfully executed.
-bool runToolOnCodeWithArgs(clang::FrontendAction *ToolAction, const Twine &Code,
-                           const std::vector<std::string> &Args,
-                           const Twine &FileName = "input.cc");
+bool runToolOnCodeWithArgs(
+    clang::FrontendAction *ToolAction, const Twine &Code,
+    const std::vector<std::string> &Args, const Twine &FileName = "input.cc",
+    const FileContentMappings &VirtualMappedFiles = FileContentMappings());
 
 /// \brief Builds an AST for 'Code'.
 ///

lib/ASTMatchers/Dynamic/Registry.cpp

   REGISTER_MATCHER(isExpr);
   REGISTER_MATCHER(isExternC);
   REGISTER_MATCHER(isImplicit);
+  REGISTER_MATCHER(isExpansionInFileMatching);
+  REGISTER_MATCHER(isExpansionInMainFile);
   REGISTER_MATCHER(isInstantiated);
+  REGISTER_MATCHER(isExpansionInSystemHeader);
   REGISTER_MATCHER(isInteger);
   REGISTER_MATCHER(isIntegral);
   REGISTER_MATCHER(isInTemplateInstantiation);

   REGISTER_MATCHER(to);
   REGISTER_MATCHER(tryStmt);
   REGISTER_MATCHER(type);
+  REGISTER_MATCHER(typedefDecl);
   REGISTER_MATCHER(typedefType);
   REGISTER_MATCHER(typeLoc);
   REGISTER_MATCHER(unaryExprOrTypeTraitExpr);

lib/Tooling/Tooling.cpp

 
 bool runToolOnCodeWithArgs(clang::FrontendAction *ToolAction, const Twine &Code,
                            const std::vector<std::string> &Args,
-                           const Twine &FileName) {
+                           const Twine &FileName,
+                           const FileContentMappings &VirtualMappedFiles) {
+
   SmallString<16> FileNameStorage;
   StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
   llvm::IntrusiveRefCntPtr<FileManager> Files(
       new FileManager(FileSystemOptions()));
-  ToolInvocation Invocation(getSyntaxOnlyToolArgs(Args, FileNameRef), ToolAction,
-                            Files.get());
+  ToolInvocation Invocation(getSyntaxOnlyToolArgs(Args, FileNameRef),
+                            ToolAction, Files.get());
 
   SmallString<1024> CodeStorage;
   Invocation.mapVirtualFile(FileNameRef,
                             Code.toNullTerminatedStringRef(CodeStorage));
+
+  for (auto &FilenameWithContent : VirtualMappedFiles) {
+    Invocation.mapVirtualFile(FilenameWithContent.first,
+                              FilenameWithContent.second);
+  }
+
   return Invocation.run();
 }
 

unittests/ASTMatchers/ASTMatchersTest.h

 using clang::tooling::newFrontendActionFactory;
 using clang::tooling::runToolOnCodeWithArgs;
 using clang::tooling::FrontendActionFactory;
+using clang::tooling::FileContentMappings;
 
 class BoundNodesCallback {
 public:

 };
 
 template <typename T>
-testing::AssertionResult matchesConditionally(const std::string &Code,
-                                              const T &AMatcher,
-                                              bool ExpectMatch,
-                                              llvm::StringRef CompileArg) {
+testing::AssertionResult matchesConditionally(
+    const std::string &Code, const T &AMatcher, bool ExpectMatch,
+    llvm::StringRef CompileArg,
+    const FileContentMappings &VirtualMappedFiles = FileContentMappings()) {
   bool Found = false, DynamicFound = false;
   MatchFinder Finder;
   VerifyMatch VerifyFound(nullptr, &Found);

       newFrontendActionFactory(&Finder));
   // Some tests use typeof, which is a gnu extension.
   std::vector<std::string> Args(1, CompileArg);
-  if (!runToolOnCodeWithArgs(Factory->create(), Code, Args)) {
+  if (!runToolOnCodeWithArgs(Factory->create(), Code, Args, "input.cc",
+                             VirtualMappedFiles)) {
     return testing::AssertionFailure() << "Parsing error in \"" << Code << "\"";
   }
   if (Found != DynamicFound) {

unittests/ASTMatchers/ASTMatchersTest.cpp

           .bind("data")));
 }
 
+TEST(TypeDefDeclMatcher, Match) {
+  EXPECT_TRUE(matches("typedef int typedefDeclTest;",
+                      typedefDecl(hasName("typedefDeclTest"))));
+}
+
+TEST(Matcher, IsExpansionInMainFileMatcher) {
+  EXPECT_TRUE(matches("class X {};",
+                      recordDecl(hasName("X"), isExpansionInMainFile())));
+  EXPECT_TRUE(notMatches("", recordDecl(isExpansionInMainFile())));
+  EXPECT_TRUE(matchesConditionally("#include <other>\n",
+                                   recordDecl(isExpansionInMainFile()), false,
+                                   "-isystem/", {{"/other", "class X {};"}}));
+}
+
+TEST(Matcher, IsExpansionInSystemHeader) {
+  EXPECT_TRUE(matchesConditionally(
+      "#include \"other\"\n", recordDecl(isExpansionInSystemHeader()), true,
+      "-isystem/", {{"/other", "class X {};"}}));
+  EXPECT_TRUE(matchesConditionally("#include \"other\"\n",
+                                   recordDecl(isExpansionInSystemHeader()),
+                                   false, "-I/", {{"/other", "class X {};"}}));
+  EXPECT_TRUE(notMatches("class X {};",
+                         recordDecl(isExpansionInSystemHeader())));
+  EXPECT_TRUE(notMatches("", recordDecl(isExpansionInSystemHeader())));
+}
+
+TEST(Matcher, IsExpansionInFileMatching) {
+  EXPECT_TRUE(matchesConditionally(
+      "#include <foo>\n"
+      "#include <bar>\n"
+      "class X {};",
+      recordDecl(isExpansionInFileMatching("b.*"), hasName("B")), true,
+      "-isystem/", {{"/foo", "class A {};"}, {"/bar", "class B {};"}}));
+  EXPECT_TRUE(matchesConditionally(
+      "#include <foo>\n"
+      "#include <bar>\n"
+      "class X {};",
+      recordDecl(isExpansionInFileMatching("f.*"), hasName("X")), false,
+      "-isystem/", {{"/foo", "class A {};"}, {"/bar", "class B {};"}}));
+}
+
 } // end namespace ast_matchers
 } // end namespace clang