jcai19 marked 4 inline comments as done.
jcai19 added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:27
+void CloexecPipeCheck::check(const MatchFinder::MatchResult &Result) {
+ const std::string &ReplacementText =
+ (Twine("pipe2(") + getSpellingArg(Result, 0) + ", O_CLOEXEC)").str();
----------------
george.burgess.iv wrote:
> simplicity nit: can this be a `std::string`?
replaceFunc takes a llvm::StringRef as the third parameter. StringRef is
"expected to be used in situations where the character data resides in some
other buffer, whose lifetime extends past that of the StringRef", which is true
in this case, so I think it should be fine.
================
Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h:18
+
+/// accept() is better to be replaced by accept4().
+///
----------------
george.burgess.iv wrote:
> nit: should probably update this comment
Good catch!
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst:4
+android-cloexec-pipe
+======================
+
----------------
Eugene.Zelenko wrote:
> Please make same length as name above.
Thanks for the comments. I have fixed them accordingly.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61967/new/
https://reviews.llvm.org/D61967
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
