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