[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
Kristóf Umann via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 16 05:45:38 PDT 2019
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";
> 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.
CHANGES SINCE LAST ACTION
More information about the cfe-commits