hokein accepted this revision.
hokein added a comment.
The code looks good to me now, I'd wait to see whether @alexfh has further
comments before submitting the patch. Thanks for your patience with the review
;)
================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:61
+ SourceLocation EndLoc =
+ Lexer::getLocForEndOfToken(FlagArg->getLocEnd(), 0, SM, getLangOpts());
+
----------------
yawanng wrote:
> hokein wrote:
> > Instead of using getLangOpts(), you should use
> > `Result.Context.getLangOpts()`.
> May I ask what's the difference between the `Result.Context.getLangOpts()`
> and `getLangOpts()`? Does `Result.Context.getLangOpts()` have a longer
> lifetime than `getLangOpts()`?
IMO they are the same fundamentally, except `Result.Context.getLangOpts()` can
save a copy. We usually use `Result.Context.getLangOpts()` inside
`ClangTidyCheck::check(const MatchFinder::MatchResult &Result)`.
================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:79
+ std::pair<FileID, unsigned> ExpansionInfo = SM.getDecomposedLoc(Loc);
+ auto MacroName =
+ SM.getBufferData(ExpansionInfo.first)
----------------
yawanng wrote:
> hokein wrote:
> > You could use `Lexer::getImmediateMacroName(Loc, SM, Opts);` to get the
> > marco name, or doesn't it work here?
> `getImmediateMacroName` sometimes cannot return the `O_CLOEXEC `, like
>
> #define O_CLOEXEC _O_CLOEXEC
> #define _O_CLOEXEC 1
>
> `getImmediateMacroName` will return `_O_CLOEXEC`, whereas `O_CLOEXEC` is what
> we need. I change this to directly get the source text if it's a macro,
> which seems to be simpler.
Ah, I see, thanks for the clarification!
================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:22
+namespace {
+bool checkFlags(const Expr *Flags, const SourceManager &SM,
+ const LangOptions &LangOpts) {
----------------
`checkFlags` name is a bit of ambiguous, maybe rename it to
"HasCloseOnExecFlag"?
Notice that you use the string "O_CLOEXEC" several times in the code, I'd
suggest to use a "CloseOnExecFlag" variable.
================
Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:33
+
+ return (MacroName == "O_CLOEXEC");
+ }
----------------
Nit: The outermost () is not needed.
https://reviews.llvm.org/D33304
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
