yawanng added a comment.
https://reviews.llvm.org/rL310669 is the latest and complete version. It fixes
an issue in windows platform.
https://reviews.llvm.org/D35372
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
yawanng updated this revision to Diff 110606.
yawanng marked 3 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
alexfh accepted this revision.
alexfh added a comment.
This revision is now accepted and ready to land.
LG with a few nits.
Comment at: clang-tidy/android/CloexecCheck.cpp:49
+ Finder->addMatcher(
+ ast_matchers::callExpr(
+ ast_matchers::callee(
yawanng updated this revision to Diff 110443.
yawanng marked 5 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
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
yawanng added a comment.
In https://reviews.llvm.org/D35372#834238, @hokein wrote:
> Looks good to me, a few nits. Thanks for improving it continuously.
>
> I'd hold it for a while to see whether @alexfh has further comments before
> submitting it.
Thank you for the reviewing :-)
yawanng updated this revision to Diff 110065.
yawanng marked 4 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
hokein accepted this revision.
hokein added a comment.
Looks good to me, a few nits. Thanks for improving it continuously.
I'd hold it for a while to see whether @alexfh has further comments before
submitting it.
Comment at: clang-tidy/android/CloexecCheck.h:28
+/// prevent
yawanng added inline comments.
Comment at: clang-tidy/android/CloexecCheck.h:72
+ StringRef getSpellingArg(const ast_matchers::MatchFinder::MatchResult
,
+ int n);
+
hokein wrote:
> `n` should be capital `N`. I think we can make it a
yawanng updated this revision to Diff 109778.
yawanng marked 13 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
hokein added inline comments.
Comment at: clang-tidy/android/CloexecCheck.cpp:66
+ // Check if the may be in the mode string.
+ const auto ModeStr = dyn_cast(ModeArg->IgnoreParenCasts());
+ if (!ModeStr || (ModeStr->getString().find(Mode) != StringRef::npos))
yawanng added inline comments.
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.
hokein wrote:
> It is unclear to me what the "issue" means here? I assume
yawanng updated this revision to Diff 109589.
yawanng marked 9 inline comments as done.
https://reviews.llvm.org/D35372
Files:
clang-tidy/android/AndroidTidyModule.cpp
clang-tidy/android/CMakeLists.txt
clang-tidy/android/CloexecCheck.cpp
clang-tidy/android/CloexecCheck.h
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("func");
yawanng added a comment.
Ping.
https://reviews.llvm.org/D35372
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
15 matches
Mail list logo
