aaron.ballman added inline comments.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:21
+
+static std::string exprToStr(const Expr *Expr,
+ const MatchFinder::MatchResult &Result) {
----------------
Do not name the parameter with the same name as a type. Same applies elsewhere
in this file.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:21
+
+static std::string exprToStr(const Expr *Expr,
+ const MatchFinder::MatchResult &Result) {
----------------
aaron.ballman wrote:
> Do not name the parameter with the same name as a type. Same applies
> elsewhere in this file.
Why returning a `std::string` rather than a `StringRef`? It seems like the
callers don't always need the extra copy operation.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:36
+ const MatchFinder::MatchResult &Result) {
+ auto NewExpr = const_cast<clang::Expr *>(Expr);
+ ParenExpr *PE = new (Result.Context)
----------------
Can you sink the `const_cast` into the only place it's required, and remove the
`clang::` from it?
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:40
+ NewExpr->getLocEnd().getLocWithOffset(1), NewExpr);
+ return Expr == PE->getSubExpr();
+}
----------------
This seems really inefficient -- it's creating a new `ParentExpr` just to throw
it away each time. Also, I'm not certain what it's accomplishing. The
constructor for `ParenExpr` accepts an `Expr *` (in this case, `Expr`) which it
returns from `ParenExpr::getSubExpr()`, so this looks like it's a noop that
just tests `Expr == Expr`. What have I missed?
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:70
+ const auto LengthCall =
+ callExpr(callee(functionDecl(hasAnyName("strlen", "wcslen"))),
+ expr().bind("length-call"));
----------------
Please match on `::strlen` and `::wcslen` so that you don't accidentally catch
random other function declarations.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:132-133
+
+ // clang-format off
+ const auto Alloca = Matcher("alloca", 1, 0, StrLenKind::WithoutInc);
+ const auto Calloc = Matcher("calloc", 2, 0, StrLenKind::WithoutInc);
----------------
Please use the fully-qualified names here as well. Also, I'm not keen on
needing to shut off clang-format just to get the alignment to work below -- we
don't typically try to keep things like this aligned unless there's a reason to
need it (like making the code maintainable), which I don't think applies here.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:193
+ diag(Result.Nodes.getNodeAs<CallExpr>("expr")->getLocStart(),
+ "'%0' function's allocated memory cannot hold the null-terminator")
+ << Name;
----------------
`null terminator` (remove the hyphen).
Also, it might be nice to reformulate to drop the possessive apostrophe on
"function's". Perhaps something like: `"memory allocated by '%0' is
insufficient to hold the null terminator"`
Same elsewhere in this file.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:210
+ auto Diag = diag(Result.Nodes.getNodeAs<CallExpr>("expr")->getLocStart(),
+ "'%0' function's result is not null-terminated")
+ << Name;
----------------
Similar here: `"the result from calling '%0' is not null-terminated"` (or
something along those lines). Same elsewhere.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:226
+ DiagnosticBuilder &Diag) {
+ if (getLangOpts().CPlusPlus11) {
+ StringRef NewFuncName = (Name[0] != 'w') ? "strncpy_s" : "wcsncpy_s";
----------------
What about C?
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:306-307
+ auto Diag = diag(Result.Nodes.getNodeAs<CallExpr>("expr")->getLocStart(),
+ "'strerror_s' function's result is not null-terminated and "
+ "missing the last character of the error message");
+ lengthArgInsertIncByOne(1, Result, Diag);
----------------
Similar rewording here to remove the possessive.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:321
+ auto Diag = diag(FuncExpr->getLocStart(),
+ "'%0' function's comparison's length is too long")
+ << Name;
----------------
Given that this is a concern about the argument to the comparison function, I
think this diagnostic should point to the argument rather than the function,
and then it can drop the name of the function entirely to alleviate the awkward
wording. Something like `"comparison length is too long and might lead to a
buffer overflow"`.
================
Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.h:59
+ void
+ lengthArgInsertIncByOne(const int ArgPos,
+ const ast_matchers::MatchFinder::MatchResult &Result,
----------------
No need for the argument to be a `const int` (same elsewhere) as the argument
will be copied regardless.
https://reviews.llvm.org/D45050
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
