[PATCH] D45468: [clang-tidy] Adding Fuchsia checker for human-readable logging

[Jonas Toth via Phabricator via cfe-commits]

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