Changeset View
Standalone View
clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp
- This file was added.
| //===--- CloexecPipeCheck.cpp - clang-tidy---------------------------------===// | |||||
| // | |||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | |||||
| // See https://llvm.org/LICENSE.txt for license information. | |||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||||
| // | |||||
| //===----------------------------------------------------------------------===// | |||||
| #include "CloexecPipeCheck.h" | |||||
| #include "clang/AST/ASTContext.h" | |||||
| #include "clang/ASTMatchers/ASTMatchFinder.h" | |||||
| using namespace clang::ast_matchers; | |||||
| namespace clang { | |||||
| namespace tidy { | |||||
| namespace android { | |||||
| void CloexecPipeCheck::registerMatchers(MatchFinder *Finder) { | |||||
| registerMatchersImpl(Finder, | |||||
| functionDecl(returns(isInteger()), hasName("pipe"), | |||||
| hasParameter(0, hasType(pointsTo(isInteger()))))); | |||||
| } | |||||
george.burgess.iv: We probably don't want to commit commented out code | |||||
| void CloexecPipeCheck::check(const MatchFinder::MatchResult &Result) { | |||||
| std::string ReplacementText = | |||||
| (Twine("pipe2(") + getSpellingArg(Result, 0) + ", O_CLOEXEC)").str(); | |||||
Not Done ReplyInline Actionssimplicity nit: can this be a std::string? george.burgess.iv: 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. jcai19: replaceFunc takes a llvm::StringRef as the third parameter. StringRef is "expected to be used… | |||||
Both should be functionally equivalent in this code. My point was that it's not common to rely on temporary lifetime extension when writing the non-const& type would be equivalent (barring maybe cases with auto, but that's not applicable), and I don't see why we should break with that here. george.burgess.iv: Both should be functionally equivalent in this code. My point was that it's not common to rely… | |||||
Make sense. jcai19: Make sense. | |||||
| replaceFunc( | |||||
| Result, | |||||
| "prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes", | |||||
| ReplacementText); | |||||
the message doesn't seem to explain the reason, especially to the people who are not familiar with the pipe API, maybe prefer pipe2() to pipe() to avoid file descriptor leakage is clearer? Ah, it looks like you are following the existing CloexecCreatCheck check, no need to do it in this patch. hokein: the message doesn't seem to explain the reason, especially to the people who are not familiar… | |||||
Not Done ReplyInline Actions"prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes" No need to change in this patch if you're following an existing pattern, but please do update the message in all checks in a separate patch to explain things better. gribozavr: "prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes"
No need… | |||||
Thanks for the comments. I have updated the text. jcai19: Thanks for the comments. I have updated the text. | |||||
| } | |||||
| } // namespace android | |||||
| } // namespace tidy | |||||
| } // namespace clang | |||||
We probably don't want to commit commented out code