[PATCH] D60107: [analyzer] NoStoreFuncVisitor: Suppress bug reports with no-store in system headers.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.
This revision is now accepted and ready to land.

It is very good to try one improvement in another similar function.



================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:364
       QualType T = PVD->getType();
-      while (const MemRegion *R = S.getAsRegion()) {
-        if (RegionOfInterest->isSubRegionOf(R) && !isPointerToConst(T))
-          return notModifiedDiagnostics(N, {}, R, ParamName,
-                                        ParamIsReferenceType, IndirectionLevel);
+      while (const MemRegion *MR = S.getAsRegion()) {
+        if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
----------------
`R` was cool, that is our unspoken naming convention. What about `CallR`/`CallRegion` and possibly `CallSVal`? If I am right usually the `BugReport` object is named `BR` because of our regions.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:547
 
   /// \return Diagnostics piece for region not modified in the current function.
   std::shared_ptr<PathDiagnosticPiece>
----------------
Update that comment?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:552
+                     const MemRegion *MatchedRegion, StringRef FirstElement,
+                     bool FirstIsReferenceType, unsigned IndirectionLevel) {
+    // Optimistically suppress uninitialized value bugs that result
----------------
What about `tryToEmitNote()`? This 'or' condition is very uncommon for a function name. Also there is another `R` what could be `BR`.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:567
+      // other functions that have more paths within them, this suppression
+      // would still apply when we visit these inner functions.
+      if (N->getStackFrame()->getCFG()->size() > 3)
----------------
The Phabricator comment is better because it has a cool example. What about merging that into this one?


================
Comment at: clang/test/Analysis/no-store-suppression.cpp:3
+
+// expected-no-diagnostics
+
----------------
Could you inject a link for the diff or copy the information for further improvements why no diagnostic happen?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60107