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