[libc-commits] [PATCH] D77281: [libc] Add libc-tidy.

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/utils/LibcTidy/EntrypointNameCheck.h:70
+  MatchFinder &finder;
+  const llvm::StringSet<> publicFunctions;
+};
----------------
Presumably an expensive copy can this be a reference?


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:112
+  // Checks
+  std::vector<std::shared_ptr<LibcTidyCheck>> checks;
+
----------------
PaulkaToast wrote:
> abrachet wrote:
> > Why are these `shared_ptr` and not `unique_ptr`?
> > 
> > We could also initialize like `std::vector<...> checks {std::make_shared<CalleeNamespaceCheck>(), ... };` instead of calling push_back twice.
> AFAIK they can't be `unique_ptr` because the checks lifetime need to extend until the end of `Tool.run()`, but we'd have to hand over ownership when calling `check-registerPPCallbacks()` this is why I went with the `shared_ptr` instead. I'm not sure if this is the best approach but I'm open to suggestions.
> 
> Also thank you this is a great pattern and eliminates the `push_back()`. (:
I think it should be fine to use `unique_ptr` and then just `std::move(checks)` along with using `auto &` in a few places


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:32
+
+llvm::cl::opt<std::string> TableGenFilePathOption(
+    "tablegen-filepath",
----------------
Is there a reason these aren't static?


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:103-105
+  // TODO: argv needs to be const for tooling's OptionParser above but
+  // TableGenMain expects a regular char*. There might be a cleaner way
+  // to grab argv[0] as a plain char* from the const version above...
----------------
You can pass non const to const. So making `argv` just `char **` and then we don't need to copy into the string should work I believe.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:116
+
+  for (auto check : checks)
+    check->registerMatchers(finder);
----------------
`auto &` wether we can use `unique_ptr` or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281