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