[PATCH] D35372: [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Sorry for the delay, and thanks for refining it.

In general, I'm fine with the current design, a few comments below.



================
Comment at: clang-tidy/android/CloexecCheck.cpp:62
+    const int ArgPos) {
+  const auto MatchedCall = Result.Nodes.getNodeAs<CallExpr>("func");
+  const auto FD = Result.Nodes.getNodeAs<FunctionDecl>("funcDecl");
----------------
Use `const auto*`, obviously indicates this is a pointer, the same to other places. 


================
Comment at: clang-tidy/android/CloexecCheck.cpp:94
+  StringRef SR = cast<StringLiteral>(Arg->IgnoreParenCasts())->getString();
+  return ("\"" + SR + Twine(Mode) + "\"").str();
+}
----------------
I think there will be a use-after-free issue here.

The method returns a StringRef (which doesn't own the "std::string"), the temporary `std::string` will be destructed after the method finished.


================
Comment at: clang-tidy/android/CloexecCheck.h:18
+
+/// The base class for all close-on-exec checks.
+class CloexecCheck : public ClangTidyCheck {
----------------
This base class will  be used in a bunch of cloase-on-exec checks.
It deserves a better and more detailed documentation (e.g. the intended usage).


================
Comment at: clang-tidy/android/CloexecCheck.h:26
+  template <typename... Types>
+  void doRegisterMatchers(ast_matchers::MatchFinder *Finder, Types... Params) {
+    auto FuncDecl = std::bind(ast_matchers::functionDecl,
----------------
I think we can drop the `do` for most methods, how about 

`doRegisterMatchers` => `registerMatchers`
`doMacroFlagInsertion` => `insertMacroFlag`
`doFuncReplacement` => `replaceFunc`
`doStringFlagInsertion` => `insertStringFlag`?


================
Comment at: clang-tidy/android/CloexecCheck.h:30
+    Finder->addMatcher(ast_matchers::callExpr(
+                           ast_matchers::callee(FuncDecl().bind("funcDecl")))
+                           .bind("func"),
----------------
Instead of using the magic string `funDecl` anywhere, I would define a static const member for it. The same to `func`.


================
Comment at: clang-tidy/android/CloexecCheck.h:35
+
+  // This issue has three types.
+  // Type1 is to insert the necessary macro flag in the flag argument.
----------------
It is unclear to me what the "issue" means here? I assume there are three types of fixes?


================
Comment at: clang-tidy/android/CloexecCheck.h:36
+  // This issue has three types.
+  // Type1 is to insert the necessary macro flag in the flag argument.
+  void
----------------
Deserve a document on the behavior of the method, would be better to give an example. The same below.

Is `Flag` required to be a macro flag? looks like it could be any string.


================
Comment at: clang-tidy/android/CloexecCheck.h:53
+  // Helper function to get the spelling of a particular argument.
+  StringRef getArgSpelling(const ast_matchers::MatchFinder::MatchResult &Result,
+                           int n);
----------------
Maybe getSpellingArg which sounds more natural?


================
Comment at: clang-tidy/android/CloexecCheck.h:54
+  StringRef getArgSpelling(const ast_matchers::MatchFinder::MatchResult &Result,
+                           int n);
+
----------------
s/n/N


================
Comment at: clang-tidy/android/CloexecCheck.h:75
+  inline ast_matchers::internal::Matcher<FunctionDecl>
+  hasSockAddrPointerTypeParameter(int n) {
+    return ast_matchers::hasParameter(
----------------
should we put this method and below in the base class? They seem to be check-specific, I think? 


https://reviews.llvm.org/D35372