aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:745-746 +def warn_fortify_source_format_overflow : Warning< + "'%0' will always overflow; destination buffer has size %1," + " but format string expands to at least %2">, + InGroup<FortifySource>; ---------------- I'm fine with the current wording because it's consistent with the existing diagnostics, but I wish we would quantify the size with units in these diagnostics. (e.g., mention that this is measured in bytes as opposed to anything else). ================ Comment at: clang/lib/Sema/SemaChecking.cpp:405-406 + bool HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier &FS, + const char *startSpecifier, + unsigned specifierLen) override { + const analyze_format_string::OptionalAmount &FW = FS.getFieldWidth(); ---------------- Naming nits: `StartSpecifier` and `SpecifierLen`. It looks like `startSpecifier` isn't used and the name can probably just be dropped. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:436-437 + switch (FS.getConversionSpecifier().getKind()) { + case analyze_format_string::ConversionSpecifier::pArg: // leading 0x + Size += 3; + break; ---------------- Why is size incremented by 3 here instead of 2 as above with a leading 0x? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:448 + } + Size -= specifierLen; + return true; ---------------- Guard against wraparound? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:489 + if (auto *Format = dyn_cast<StringLiteral>(FormatExpr)) { + + if (!Format->isAscii() && !Format->isUTF8()) ---------------- Spurious whitespace. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:490 + + if (!Format->isAscii() && !Format->isUTF8()) + return; ---------------- We can handle UTF-8 format strings even when they contain non-ASCII characters? ================ Comment at: clang/lib/Sema/SemaChecking.cpp:8237 H.DoneProcessing(); + } else if (Type == Sema::FST_Scanf) { ---------------- Spurious whitespace change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71566/new/ https://reviews.llvm.org/D71566 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits