Szelethus added a comment. I really like the change, thanks! Let's settle on the diagnostic message then.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:202 + SVal l, + unsigned idx = -1u) const; ProgramStateRef CheckLocation(CheckerContext &C, ---------------- I think `Optional<unsigned>` would be nicer. Also, `checkNonNull` as a function name doesn't scream about why we need an index parameter -- could you rename it to `IndexOfArg` or something similar? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:195-196 + llvm::raw_svector_ostream OS(SBuf); + OS << "Null pointer passed as an argument to a 'nonnull' "; + OS << Idx << llvm::getOrdinalSuffix(Idx) << " parameter"; + ---------------- whisperity wrote: > It seems to be as if now you're able to present which parameter is the > `[[nonnull]]` one. Because of this, the output to the user sounds really bad > and unfriendly, at least to me. > > How about this: > > "null pointer passed to nth parameter, but it's marked 'nonnull'"? > "null pointer passed to nth parameter expecting 'nonnull'"? > > Something along these lines. > > To a parameter, we're always passing arguments, so saying "as an argument" > seems redundant. > > And because we have the index always in our hands, we don't need to use the > indefinite article. Agreed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
