[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 inline comments.


================
Comment at: clang-tidy/android/CloexecCheck.cpp:66
+  // Check if the <Mode> may be in the mode string.
+  const auto ModeStr = dyn_cast<StringLiteral>(ModeArg->IgnoreParenCasts());
+  if (!ModeStr || (ModeStr->getString().find(Mode) != StringRef::npos))
----------------
`const auto*`


================
Comment at: clang-tidy/android/CloexecCheck.h:11
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_H
+#include "../ClangTidy.h"
----------------
A blank line here.


================
Comment at: clang-tidy/android/CloexecCheck.h:23
+class CloexecCheck : public ClangTidyCheck {
+  const static constexpr char *FuncDeclBindingStr = "funcDecl";
+  const static constexpr char *FuncBindingStr = "func";
----------------
Normally, the style should like:
```
class Foo {
public:
  ...
protected:
  ...
privated:
  ...
};
```

BTW, in C++11 these are declarations, you still need to define them in the .cc file like 

```
const static constexpr char* CloexecCheck::FuncDeclBindingStr;
``` 


================
Comment at: clang-tidy/android/CloexecCheck.h:32
+  template <typename... Types>
+  void registerMatchersImpl(ast_matchers::MatchFinder *Finder,
+                            Types... Params) {
----------------
I think this method (and below) can be `protected` member.


================
Comment at: clang-tidy/android/CloexecCheck.h:51
+  void insertMacroFlag(const ast_matchers::MatchFinder::MatchResult &Result,
+                       const StringRef Flag, const int ArgPos);
+
----------------
I'd name it `MacroFlag`.


================
Comment at: clang-tidy/android/CloexecCheck.h:59
+  void replaceFunc(const ast_matchers::MatchFinder::MatchResult &Result,
+                   StringRef Msg, StringRef FixMsg);
+
----------------
It is not obvious to see what these parameter mean. `Msg` is a message for warning, maybe `WarningMsg`? 

I'd suggest using the formal documentation for this class (https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments). 


================
Comment at: clang-tidy/android/CloexecCheck.h:63
+  // the flag is some string rather than a macro. For example, 'fopen' needs
+  // string 'e' in its mode argument, so the fix should be:
+  //   fpen(in_file, "r");
----------------
`e` is not a string, is a char.


================
Comment at: clang-tidy/android/CloexecCheck.h:64
+  // string 'e' in its mode argument, so the fix should be:
+  //   fpen(in_file, "r");
+  //                ^~~
----------------
s/fpen/fopen.


================
Comment at: clang-tidy/android/CloexecCheck.h:68
+  void insertStringFlag(const ast_matchers::MatchFinder::MatchResult &Result,
+                        const StringRef Mode, const int ArgPos);
+
----------------
Is `char` for `Mode` sufficient? Seems to me that only `fopen` check is using it, or there will be more checks using this in the future?


================
Comment at: clang-tidy/android/CloexecCheck.h:72
+  StringRef getSpellingArg(const ast_matchers::MatchFinder::MatchResult &Result,
+                           int n);
+
----------------
`n` should be capital `N`. I think we can make it a const member method. And I can't see any usage of this method in this patch.


================
Comment at: clang-tidy/android/CloexecCheck.h:75
+  // Helper function to form the correct string mode for Type3.
+  std::string buildFixMsgForStringFlag(const Expr *Arg, const SourceManager &SM,
+                                     const LangOptions &LangOpts,
----------------
Looks like this method is used internally, do we need to expose it? or just define it as an anonymous function in the cc file?


================
Comment at: clang-tidy/android/CloexecCheck.h:79
+
+  inline ast_matchers::internal::Matcher<FunctionDecl>
+  hasIntegerParameter(int N) {
----------------
The `inline` is redundant. 


================
Comment at: clang-tidy/android/CloexecMemfdCreateCheck.cpp:24
+void CloexecMemfdCreateCheck::check(const MatchFinder::MatchResult &Result) {
+  insertMacroFlag(Result, "MFD_CLOEXEC", 1);
+}
----------------
nit: add `/*ArgPos=*/` before parameter 1 to make it more readable.


https://reviews.llvm.org/D35372