alexfh added a comment. I'm not sure this approach is the best to deal with the boilerplate, but it seems like an improvement compared to repeating code. See a few comments inline.
================ Comment at: clang-tidy/android/CloexecCheck.cpp:66 + +StringRef CloexecCheck::getSpellingArg(const MatchFinder::MatchResult &Result, + int N) const { ---------------- 1. Should this be a class method? 2. Is this ever used? (if it is going to be used in a different check, let's postpone its addition to that moment) ================ Comment at: clang-tidy/android/CloexecCheck.h:43 + ast_matchers::callExpr( + ast_matchers::callee(FuncDecl().bind(FuncDeclBindingStr))) + .bind(FuncBindingStr), ---------------- Can we make this method non-template and put its implementation to the .cpp file? It could accept a Matcher<FunctionDecl> or something like that and just apply it: void registerForCall(MatchFinder *Finder, Matcher<FunctionDecl> Function) { Finder->addMatcher(calExpr(callee(functionDecl(isExternC(), Function).bind(...))).bind(...), this); } Then you could make `FuncDeclBindingStr` and `FuncBindingStr` local to the file. ================ Comment at: clang-tidy/android/CloexecCheck.h:47 + } + /// Currently, we has three types of fixes. + /// ---------------- s/we has/we have/ ================ Comment at: clang-tidy/android/CloexecCheck.h:102-137 + ast_matchers::internal::Matcher<FunctionDecl> hasIntegerParameter(int N) { + return ast_matchers::hasParameter( + N, ast_matchers::hasType(ast_matchers::isInteger())); + } + + ast_matchers::internal::Matcher<FunctionDecl> + hasCharPointerTypeParameter(int N) { ---------------- These methods don't add much value: `hasParameter(N, hasType(isInteger()))` is not much longer or harder to read than `hasIntegerParameter(N)`, etc. Also having these utilities as non-static methods doesn't make much sense. If you want a shorter way to describe function signatures, I would suggest a single utility function `hasParameters()` that would take a list of type matchers and apply them to the corresponding parameters. That one could be useful more broadly than in this specific check, so it could be placed in utils/ (and later in ASTMatchers.h, if we find enough potential users). ================ Comment at: clang-tidy/android/CloexecCheck.h:140 +private: + const static char *FuncDeclBindingStr; + const static char *FuncBindingStr; ---------------- You're missing one more const: `static const char * const FuncDeclBindingStr;` https://reviews.llvm.org/D35372 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits