JonasToth added inline comments.
================
Comment at: clang-tidy/zircon/HumanReadableStatusCheck.cpp:68
+ SourceRange FmtStringRange;
+ for (const auto *Arg : D->arguments()) {
+ if (const auto *ICE = dyn_cast<ImplicitCastExpr>(Arg)) {
----------------
Did you consider using `forEachArgumentWithParam` matcher?
I think you could match any `zx_status_t` argument for the formatter functions
and bind on them.
================
Comment at: clang-tidy/zircon/HumanReadableStatusCheck.cpp:103
+
+std::string HumanReadableStatusCheck::ReplaceFmtString(int Pos,
+ StringRef FmtString) {
----------------
Is this required as member function? You can make that `static` (or move into
an unnamed namespace, not 100% sure about the ruling in llvm)
================
Comment at: clang-tidy/zircon/HumanReadableStatusCheck.cpp:108
+ while (Current <= Pos && Loc < FmtString.size() - 1) {
+ Loc = FmtString.find('%', Loc);
+ ++Current;
----------------
That does not consider `\%` and would be buggy for such fmt strings
================
Comment at: clang-tidy/zircon/HumanReadableStatusCheck.cpp:112
+ }
+ std::string ReplacementString = FmtString;
+ if (Loc != StringRef::npos && ReplacementString[Loc] == 'd')
----------------
I think this replacement strategy is not general enough.
http://www.cplusplus.com/reference/cstdio/printf/
`%[flags][width][.precision][length]specifier`
If we decide to implement an actual `printf`-fmt string replacer that could
maybe land in `utility`.
I assume that `%d` is the format specification used for 99,99% of all relevant
calls?! Maybe it is enough anyway, because the check matches on very specific
`%d` arguments.
What about the other integer formats? Any chance they are used?
================
Comment at: docs/ReleaseNotes.rst:202
+
+ FIXME: add release notes.
+
----------------
Please add a short describing sentence.
================
Comment at: docs/ReleaseNotes.rst:204
+
+- New `zircon-temporary-objects
+
<http://clang.llvm.org/extra/clang-tidy/checks/zircon-temporary-objects.html>`_
check
----------------
This seems to be slipped through.
================
Comment at: test/clang-tidy/zircon-human-readable-status.cpp:68
+ WRAP("%d, %d", notstatus, status);
+ wrapper("%d, %d", notstatus, status);
+
----------------
Please add test with escaped `%` and other crazy format strings.
How are advanced format strings treated? I think it is necessary to do better
parsing.
https://reviews.llvm.org/D45468
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
