[PATCH] D75063: [analyzer] StdLibraryFunctionsChecker: Add NotNull Arg Constraint

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D75063#1923780 <https://reviews.llvm.org/D75063#1923780>, @NoQ wrote:

> This is basically a shorthand for "outside [0, 0]", right? I don't mind ^.^


Yeah, and my first attempt was exactly to implement this with ranges. However, it failed when I realized that we cannot cast a pointer to `NonLoc`, so the already written `RangeConstraint::apply*` functions could not work (I would have to add another branch for handling `Loc` kind of SVals for the pointer case).



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:175
+    using ValueConstraint::ValueConstraint;
+    bool CannotBeNull = true;
+
----------------
Szelethus wrote:
> martong wrote:
> > Szelethus wrote:
> > > What does this do? Is it ever used in the patch?
> > Yes, it is used. We use it in `apply` the value is passed to `assume`.
> > And in `negate` we flip the value.
> Forgot my eyes in the office. Woops. I would still prefer a line of comment here :)
Ok, I added a comment about its role.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:684
+        .Case({
+            ReturnValueCondition(LessThanOrEq, ArgNo(2)),
+        })
----------------
martong wrote:
> steakhal wrote:
> > Two lines below you are using the `{0U}` initialization syntax, and here the simple constructor call syntax.
> > Shouldn't we pick one?
> Yes, definitely. I think I am going to use brace initialization syntax everywhere.
Finally I ended up with the parens. :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:889-890
                           Read(LongLongTy, LongLongMax)}},
       {"fread", Summaries{Fread()}},
-      {"fwrite", Summaries{Fread()}},
+      {"fwrite", Summaries{Fwrite()}},
       // getline()-like functions either fail or read at least the delimiter.
----------------
Szelethus wrote:
> Not super relevant to this specific revision, but shouldn't we leave these to `StreamChecker`?
Well, yeah we could remove `fread` and `fwrite` from the summaries entirely at some point, but that will require changing the test files here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75063