[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
Tibor Brunner via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Aug 17 14:50:18 PDT 2019
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,
> 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);
> 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";
> 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.
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 :)
CHANGES SINCE LAST ACTION
More information about the cfe-commits