[PATCH] D104616: [analyzer][WIP] Model comparision methods of std::unique_ptr

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:329
+      return *Ptr;
+    return C.getSValBuilder().conjureSymbolVal(
+        E, C.getLocationContext(),
----------------
In case we conjure a new symbol, we want this stored in the `TrackedRegionMap`. So the analyzer can correctly record the constraints between the two symbols. 


================
Comment at: clang/test/Analysis/smart-ptr.cpp:466
+
+  clang_analyzer_eval(ptr == ptr); // expected-warning{{TRUE}}
+  clang_analyzer_eval(ptr > ptr);  // expected-warning{{FALSE}}
----------------
RedDocMD wrote:
> xazax.hun wrote:
> > Putting tests like this on the same path can be risky. Tests might split the execution path (maybe not now, but in the future). I think it might be more future proof to have a large switch statement that switches on an unknown value and put the tests in separate cases. 
> I didn't quite get you.
You remember this in the other patch:
```
member-constructor.cpp:15:5: warning: FALSE [debug.ExprInspection]
    clang_analyzer_eval(*P->p == 0);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
member-constructor.cpp:15:25: note: Assuming the condition is false
    clang_analyzer_eval(*P->p == 0);
                        ^~~~~~~~~~
member-constructor.cpp:15:5: note: FALSE
    clang_analyzer_eval(*P->p == 0);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
member-constructor.cpp:15:5: warning: TRUE [debug.ExprInspection]
    clang_analyzer_eval(*P->p == 0);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
member-constructor.cpp:15:25: note: Assuming the condition is true
    clang_analyzer_eval(*P->p == 0);
                        ^~~~~~~~~~
member-constructor.cpp:15:5: note: TRUE
    clang_analyzer_eval(*P->p == 0);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
```

It looks like this does not happen for overloaded comparison operators at the moment. But we might want to do that in the future (@NoW what do you think). I was wondering, if we want to future proof these test cases for that behavior. But looking at the test cases again, you only have two, where the expected result is unknown, and they are at the very end. So feel free to ignore this and leave the code as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616