[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message

[Kristóf Umann via Phabricator via cfe-commits]

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