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

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Mostly mechanical changes requested here.

In D93164#2456247 <https://reviews.llvm.org/D93164#2456247>, @steveire wrote:

> @thakis FYI - I think the GN build would need to be adapted to this.

FWIW the GN build has a bot that can typically update its gn files.



================
Comment at: clang/include/clang/Tooling/NodeIntrospection.h:40
+  LocationCall *on() const { return m_on.get(); }
+  std::string name() const { return m_name; }
+  std::vector<std::string> const &args() const { return m_args; }
----------------
Is there a strong use case to return by value here?


================
Comment at: clang/include/clang/Tooling/NodeIntrospection.h:54
+public:
+  static std::string format(LocationCall *Call) {
+    std::vector<LocationCall *> vec;
----------------
Should this (and potential a few others) be moved to an implementation file.


================
Comment at: clang/include/clang/Tooling/NodeIntrospection.h:55
+  static std::string format(LocationCall *Call) {
+    std::vector<LocationCall *> vec;
+    while (Call) {
----------------
It's probably more effective to use a SmallVector to reduce the needs for allocations here.


================
Comment at: clang/include/clang/Tooling/NodeIntrospection.h:60
+    }
+    std::reverse(vec.begin(), vec.end());
+    std::string result;
----------------
This is wasteful, just operate on rbegin/rend later and eliminate this call.


================
Comment at: clang/include/clang/Tooling/NodeIntrospection.h:63
+    auto secondLast = std::prev(vec.end());
+    for (auto VecIt = vec.begin(); VecIt != secondLast; ++VecIt) {
+      auto VecCall = *VecIt;
----------------
Is this more readable, IDK, but it sure as hell is more fun :)


================
Comment at: clang/include/clang/Tooling/NodeIntrospection.h:65-66
+      auto VecCall = *VecIt;
+      result += VecCall->name() + "()";
+      result += VecCall->returnsPointer() ? "->" : ".";
+    }
----------------



================
Comment at: clang/lib/Tooling/DumpTool/APIData.h:21
+
+  bool IsEmpty() const { return Locs.empty() && Rngs.empty(); }
+
----------------
steveire wrote:
> aaron.ballman wrote:
> > per the usual naming rules.
> I changed it, but are you referring to this proposal? https://llvm.org/docs/Proposals/VariableNames.html
While there is a proposal in place, right now we should ensure we aren't deviating from the current system in patches. Unfortunately readability-identifier-naming has been disabled on clang directory due to excessive violations. May I suggest re-enabling it locally and then running clang-tidy-diff.py over the patch. should square most things up.


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:22-25
+auto publicAccessor = [](auto... InnerMatcher) {
+  return cxxMethodDecl(isPublic(), parameterCountIs(0), isConst(),
+                       InnerMatcher...);
+};
----------------
Why is this a variable, a templated function should do the same thing.
I imagine its something like this.


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.cpp:45
+
+  return Finder.release()->newASTConsumer();
+}
----------------
This seems dangerous, the `MatchASTConsumer` doesn't own its `MatchFinder`, so this is going to leak.


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.h:1
+//===- ASTSrcLocProcessor.h -----------------------------------------------===//
+//
----------------
Can you add a modeline here, same goes for other headers.


================
Comment at: clang/lib/Tooling/DumpTool/ASTSrcLocProcessor.h:14
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include "APIData.h"
----------------
Any reason for empty lines between includes?


================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:80
+
+  std::transform(IncludeDirectories.begin(), IncludeDirectories.end(),
+                 std::back_inserter(Args),
----------------



================
Comment at: clang/lib/Tooling/DumpTool/ClangSrcLocDump.cpp:86-88
+  std::vector<const char *> Argv;
+  std::transform(Args.begin(), Args.end(), std::back_inserter(Argv),
+                 [](const std::string &Arg) { return Arg.c_str(); });
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93164