njames93 added a comment.
This check needs documentation adding in
`clang-tools-extra/docs/clang-tidy/checks/linux-kernel-logfunctions-h.rst`
Also needs to add this in `clang-tools-extra/docs/clang-tidy/checks/list.rst`,
keeping it in alphabetical order.
Also need to add an entry to `clang-tools-extra/docs/ReleaseNotes.rst`
You may also need to check the state of your local branch as the pre merge
build bot can't apply this cleanly in order to test.
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:42-45
+ compoundStmt(has(
+ callExpr(has(ignoringParenImpCasts(stringLiteral().bind("lit"))),
+ callee(functionDecl(hasAttr(attr::Format)).bind("func")))
+ .bind("call"))),
----------------
Why are you matching on the `compoundStmt`, surely the `callExpr` should be the
top level matcher?
Doing this will miss cases where 2 occurance of `printf` occur in the same
`compoundStmt` and miss the cases where `printf` isn't in a `compoundStmt`.
```lang=c
if (...)
printf(...);```
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:58
+ if (Format->getType()->getName() == "printf" &&
+ ArgNum < Call->getNumArgs() &&
+ Literal == Call->getArg(ArgNum)->IgnoreCasts() && Literal->isAscii() &&
----------------
Should this be an assert, I can't think of a reason why the FormatIDx would be
out of range.
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:59
+ ArgNum < Call->getNumArgs() &&
+ Literal == Call->getArg(ArgNum)->IgnoreCasts() && Literal->isAscii() &&
+ Literal->getCharByteWidth() == 1) {
----------------
Why bother binding the `lit` node if you can just get it from the FormatArgs.
This is also potentially safer also as it won't get confused by
```lang=c
printf("fmtString", "formatArg");```
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:64
+ "%hhi", "%hhd", "%hhu", "%hhx"};
+ for (auto FoundH : LookForH) {
+ if (LiteralString.contains(FoundH)) {
----------------
This whole loop looks wrong.
It will only report the first instance of `FoundH` and its doing a lot of
duplicated work
It would be better to scan across the string for `%h` then check the next chars
for `[idux]` or `h[idux]`
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:65
+ for (auto FoundH : LookForH) {
+ if (LiteralString.contains(FoundH)) {
+ size_t HBegin = LiteralString.find(FoundH) + 1;
----------------
Can this be refactored to use an early exit instead.
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:79
+ Result.Context->getLangOpts(), Result.Context->getTargetInfo());
+ diag(EndLoc, "Integer promotion: Using '%0' in '%1' is unnecessay")
+ << ErrorString << FoundH
----------------
Why is the diag location reported as the end of the format specifier, It should
really be pointed to the start of the format specifier or location of the `h`.
```lang=c
printf("%hx, a);
^
printf("%hx, a);
^
```
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.cpp:90
+void LogFunctionsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "Fixer", "");
+}
----------------
There is no option retrieved called `Fixer`, so there should be no option
stored call `Fixer`.
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.h:13-14
+#include "../ClangTidyCheck.h"
+#include "clang/Lex/MacroInfo.h"
+#include <vector>
+
----------------
These includes seem redundant in this file.
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/LogFunctionsCheck.h:22
+
+ enum LogFunctionsFixerKind { LFFK_None, LFFK_H };
+
----------------
Right now you are only using `LFFK_H`. So for simplicity, this enum should be
removed from this patch. If there is a follow up where this actually used, the
enum should be added there.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/linuxkernel-logfunctions-h.c:3
+
+extern printk(const char *, ...) __attribute__((format(printf, 1, 2)));
+
----------------
Can you add a check with a function
```lang=c
extern printf(const char *, ...);
```
Just to demonstrate it will only flag functions that have the printf format
attribute.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93182/new/
https://reviews.llvm.org/D93182
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
