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

[Tibor Brunner via Phabricator via cfe-commits]

bruntib marked 3 inline comments as done.
bruntib added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:202
+                                   SVal l,
+                                   unsigned idx = -1u) const;
   ProgramStateRef CheckLocation(CheckerContext &C,
----------------
Szelethus wrote:
> 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?
The default value of IdxOfArg comes from my laziness. There were two places where I was not sure what index number should be given, but I added those too. This way no default value or Optional is needed. Omitting this information wouldn't even be reasonable, since there is always an ordinal number of the argument.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1548
 
-  state = checkNonNull(C, state, Dst, DstVal);
+  state = checkNonNull(C, state, Dst, DstVal, 1);
   if (!state)
----------------
NoQ wrote:
> You could also pass a description of the parameter (eg., "source" or "destination").
Could you please give some hint, how to include this in the message? I don't know how to do it concisely.


================
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";
+
----------------
Szelethus wrote:
> 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.
I used the original message and just extended it with a number. Are you sure that it is a good practice to change a checker message? I'm not against the modification, just a little afraid that somebody may rely on it. It's quite unlikely though, so I changed it :) 


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66333/new/

https://reviews.llvm.org/D66333