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

[Paula Toth via Phabricator via libc-commits]

PaulkaToast added inline comments.


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:55
+
+//-- Check Callbacks --//
+class QualifiedCallCheck : public MatchFinder::MatchCallback {
----------------
abrachet wrote:
> What is the purpose of this and the end comment?
I added these comments to give us a place to add additional checks when the time comes, since I don't see us having so many that we need to put them in their own folder/files but we will have more than one. 


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:76
+    // If the call is prefixed with the qualified __llvm_libc:: it's good to go.
+    if (funcDecl->getQualifiedNameAsString().rfind("__llvm_libc::", 0) == 0)
+      return;
----------------
miscco wrote:
> `rfind` searches for the last occurrence of the sub-string. Do you expect multiple qualifiers?
No we don't, I changed this to use StringRef's startswith since its much more readable. thanks!


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:83
+    DiagnosticsEngine &DE = result.Context->getDiagnostics();
+    const unsigned ID = DE.getCustomDiagID(
+        DiagnosticsEngine::Error,
----------------
abrachet wrote:
> No point for `const`
Ah, sorry I might be missing something. What is the reasoning for not using const here? I thought it's good practice to use const anytime you don't plan to modify the variable? To prevent mistakes like `ID += 1;` as this compiles but yields runtime errors. 


================
Comment at: libc/utils/LibcTidy/LibcTidyTool.cpp:102-104
+  if (llvm::Error Err = functionsOrErr.takeError()) {
+    return 1;
+  }
----------------
abrachet wrote:
> `logAllUnhandledErrors(std::move(Err), errs())` here probably.
I change the implementation to utilize TableGenMain instead so this shouldn't be an issue anymore.


================
Comment at: libc/utils/LibcTidy/TableGenUtil.cpp:48-50
+    for (llvm::StringRef functionName : functionNameList) {
+      funcNames.insert(std::string(functionName));
+    }
----------------
abrachet wrote:
> `llvm::transform(functionNameList, std::back_inserter(funcNames), [](StringRef s) { return std::string(s); });` maybe
I think this is a little too fancy for me.  Don't get me wrong I love it, just difficult to read. (:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77281