[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:483
+                                            llvm::raw_ostream &OS) {
+                                 BR.markInteresting(ThisRegion);
+                                 OS << "Smart pointer";
----------------
Wait, why are we marking the region interesting here? Not every null smart pointer is interesting, only the ones that are actually dereferenced.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:497-499
+                                 OS << "Smart pointer";
+                                 checkAndPrettyPrintRegion(OS, ThisRegion);
+                                 OS << " is non-null";
----------------
Also i'm starting to suspect that these notes aren't actually needed when there's no "Assuming..."; in particular, we don't emit such notes for raw pointers, so we probably shouldn't emit them for smart pointers either. There's anyway going to be a note about "Taking true branch..." or something like that, which is sufficient to understand what was the smart pointer.

I.e., this note is unnecessary:
```lang=c++
void foo() {
  std::unique_ptr<int> P = nullptr;
  if (P) {        // Smart pointer 'P' is null
                  // Taking false branch
  }
}
```
The second note, "Taking false branch", is sufficient for explaining to the user that the smart pointer is known to be null. The user may ask why the smart pointer is null, but the note isn't explaining it. The way you marked the region as interesting (as i noticed in the above inline comment) would have indeed explained why it's null, but at this point we draw the line and believe that if the region isn't already interesting then the user most likely doesn't need to know why it's null (and if it's already interesting then there's no need to mark it again).

But this note is necessary:
```lang=c++
void foo(std::unique_ptr<int> P) {
  if (P) {        // Assuming smart pointer 'P' is null
                  // Taking false branch
  }
}
```
This note conveys the extra information that we don't know for a fact that the smart pointer is null on the current path based on previous analysis, but instead the analyzer is *assuming* that it's possible that it's null due to the presence of the check in the code. That's a big deal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86027