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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.


================
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}}
----------------
xazax.hun wrote:
> 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.
Then perhaps I should leave a comment to indicate this possibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104616