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  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 

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.


cfe-commits mailing list

Reply via email to