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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
