[PATCH] D73898: [analyzer] StdLibraryFunctionsChecker: Add argument constraints

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

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

LGTM, aside from some checker tagging nightmare. Its a bit easy to mess up, please allow me to have a final look before commiting! :)



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:298
 
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions, "
----------------
Just noticed, this checker still lies in the `apiModeling` package. Could we find a more appropriate place?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:92-93
 
+  class ValueConstraint;
+  using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
   class ValueConstraint {
----------------
martong wrote:
> Szelethus wrote:
> > How about `ValueConstraintRef`?
> Yeah, we have `ProgramStateRef` and `SymbolRef`. And both are actually just synonyms to smart pointers. I'd rather not call a pointer as a reference, because that can be confusing when reading the code. E.g. when I see that we return with a `nullptr` from a function that can return with a `...Ref` I start to scratch my head.
Sure, I'm sold.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:260
 
+  BugType BT{this, "Unsatisfied argument constraints", categories::LogicError};
+
----------------
By passing `this`, the error message will be tied to the modeling checker, not to the one you just added. `BugType` has a constructor that accepts a string instead, pass `CheckNames[CK_StdCLibraryFunctionArgsChecker]` in there :) Also, how about `BT_InvalidArgument` or something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898