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