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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
