[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";
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.

  rC Clang



More information about the cfe-commits mailing list