[PATCH] D125400: [clang][Analyzer] Add errno state to standard functions modeling.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Nice work! Could you pleas add some lit tests that describe an errno related bugreport for a standard lib function?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:381
 
+  class ErrnoConstraintKind {
+  public:
----------------
This class serves as a base class, it is not really a `Kind` class which is usually just an enum.
I'd prefer to call this `ErrnoConstraintBase` or `BasicErrnoConstraint`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:383
+  public:
+    virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
+                                  const Summary &Summary,
----------------
This class is a polymorphic base class, since we have this virtual function here. Would make sense to make destructor virtual as well, even though delete is not called explicitly on the base class (as I see).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:397
+
+  class SingleValueErrnoConstraint : public ErrnoConstraintKind {
+    uint64_t const ErrnoValue;
----------------
Please document the subclasses.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:603
 
-    Summary &Case(ConstraintSet &&CS, StringRef Note = "") {
-      Cases.push_back(SummaryCase(std::move(CS), Note));
+    Summary &Case(ConstraintSet &&CS, const ErrnoConstraintKind &ErrnoC,
+                  StringRef Note = "") {
----------------
Would it make sense to have a `ErrnoIrrelevant` as the default value for `ErrnoC`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:719-722
+  NoErrnoConstraint ErrnoIrrelevant;
+  SuccessErrnoConstraint ErrnoMustNotBeChecked;
+  ZeroRelatedErrnoConstraint ErrnoNEZeroIrrelevant{
+      clang::BinaryOperatorKind::BO_NE, errno_modeling::Errno_Irrelevant};
----------------
Could you please add some more documentation to these variables? And they could be `const`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125400