[PATCH] D93164: [AST] Add generator for source location introspection

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Do I understand correctly that the workflow is to use the new dumping tool to generate the needed JSON file that then gets used as input to generate_cxx_src_locs.py which creates NodeLocationIntrospection.cpp/.h that then gets used by clang-query (eventually)? So there are two levels of translation involved to get the final source code? If so, do you know what the performance/overhead for this looks like compared to a typical build? I'm trying to get an idea for whether this will have negative impacts on the build bots such that we may want to add an LLVM cmake configure option to control whether this work happens or not.



================
Comment at: clang/lib/Tooling/DumpTool/APIData.h:1
+//===- srclocdumper.cpp --------------------------------------------===//
+//
----------------
Looks like a copy pasta error.


================
Comment at: clang/lib/Tooling/DumpTool/APIData.h:10
+
+#ifndef LLVM_CLANG_TOOLING_APIDATA_H
+#define LLVM_CLANG_TOOLING_APIDATA_H
----------------
Might as well fix this lint warning.


================
Comment at: clang/lib/Tooling/DumpTool/APIData.h:21
+
+  bool IsEmpty() const { return Locs.empty() && Rngs.empty(); }
+
----------------
per the usual naming rules.


================
Comment at: clang/lib/Tooling/DumpTool/APIData.h:25
+  std::vector<std::string> Rngs;
+  // TODO: Extend this with locations available via typelocs etc.
+};
----------------
Are these extensions going to add new members for those? If so, perhaps `Locs` and `Rngs` should have more descriptive names initially?


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:40
+              // TODO: Extend this with other clades
+              namedDecl(hasName("clang::Stmt")).bind("nodeClade")),
+          optionally(isDerivedFrom(cxxRecordDecl().bind("derivedFrom"))))
----------------
TIL what "clade" means, thank you for that new word. :-D


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:71-74
+  if (!Obj.Locs.empty())
+    JsonObj["locs"] = Obj.Locs;
+  if (!Obj.Rngs.empty())
+    JsonObj["rngs"] = Obj.Rngs;
----------------
Similar question here about whether we should use less generic names or not.


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:125
+  for (const auto &BN : BoundNodesVec) {
+    if (const auto Node = BN.getNodeAs<clang::NamedDecl>("classMethod")) {
+      // Only record the getBeginLoc etc on Stmt etc, because it will call
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:127
+      // Only record the getBeginLoc etc on Stmt etc, because it will call
+      // more-derived implementations pseudo-virtually
+      if ((ASTClass->getName() != "Stmt" && ASTClass->getName() != "Decl") &&
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:134
+      // Only record the getExprLoc on Expr, because it will call
+      // more-derived implementations pseudo-virtually
+      if ((ASTClass->getName() != "Expr") &&
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:135-136
+      // more-derived implementations pseudo-virtually
+      if ((ASTClass->getName() != "Expr") &&
+          (Node->getName() == "getExprLoc")) {
+        continue;
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:154-155
+
+    auto NodeClade = Result.Nodes.getNodeAs<clang::CXXRecordDecl>("nodeClade");
+    auto CladeName = NodeClade->getName();
+
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:157
+
+    if (auto DerivedFrom =
+            Result.Nodes.getNodeAs<clang::CXXRecordDecl>("derivedFrom"))
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.h:10
+
+#ifndef LLVM_CLANG_TOOLING_ASTSRCLOCPROCESSOR_H
+#define LLVM_CLANG_TOOLING_ASTSRCLOCPROCESSOR_H
----------------
This one as well.


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.h:17
+
+#include <map>
+#include <string>
----------------
I don't think this is being used, but you should include what you use (`StringRef`, `unique_ptr`)


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.h:29
+public:
+  ASTSrcLocProcessor(StringRef JsonPath);
+
----------------
Should this ctor be marked `explicit`?


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.h:31-34
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
+                                                 StringRef File);
+
+  void Generate();
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:43
+
+static cl::opt<std::string> JsonOutputPath("json-output-path",
+                                           cl::desc("json output path"),
----------------
Hmmm, do we want such a long name for this option? I was guessing that `-o` isn't appropriate because there may be multiple files output and so this isn't naming the output file but the output directory, but `json-output-path` is a mouthful for setting the output location. Or do you think users won't be using that option often enough for the length to matter?


================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:70
+    std::error_code EC;
+    llvm::raw_fd_ostream jsonOut(JsonOutputPath, EC, llvm::sys::fs::F_Text);
+    if (EC)
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:73
+      return 1;
+    jsonOut << formatv("{0:2}", llvm::json::Value(llvm::json::Object()));
+    return 0;
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:105
+  auto Driver = std::make_unique<driver::Driver>(
+      "clang++", llvm::sys::getDefaultTargetTriple(), Diagnostics,
+      "ast-api-dump-tool", &Files.getVirtualFileSystem());
----------------
Do you think we may need the ability for the user to pass other options to this driver invocation for things like `-fms-compatibility` or whatnot?


================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:108
+
+  const auto Compilation(Driver->BuildCompilation(llvm::makeArrayRef(Argv)));
+  if (!Compilation)
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:112
+
+  const auto &Jobs = Compilation->getJobs();
+  if (Jobs.size() != 1 || !isa<driver::Command>(*Jobs.begin())) {
----------------



================
Comment at: clang/lib/Tooling/DumpTool/generate_cxx_src_locs.py:76-86
+    if (LHS.first.getBegin() < RHS.first.getBegin())
+      return true;
+    else if (LHS.first.getBegin() != RHS.first.getBegin())
+      return false;
+
+    if (LHS.first.getEnd() < RHS.first.getEnd())
+      return true;
----------------
Would this be a more clear equivalent?


================
Comment at: clang/unittests/Introspection/IntrospectionTest.cpp:14
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
----------------
Might as well fix this lint warning.


================
Comment at: clang/unittests/Introspection/IntrospectionTest.cpp:40
+
+  auto FooCall = BoundNodes[0].getNodeAs<CallExpr>("fooCall");
+
----------------



================
Comment at: clang/unittests/Introspection/IntrospectionTest.cpp:64
+
+  auto FooCall = BoundNodes[0].getNodeAs<CallExpr>("fooCall");
+
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93164/new/

https://reviews.llvm.org/D93164