[PATCH] D75514: [Analyzer] Only add container note tags to the operations of te affected container

Balázs Benics via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Mar 3 09:06:06 PST 2020


steakhal added a comment.

I don't have any technical comments on this patch since I haven't used `NoteTags` yet, only a couple of readability ones.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp:97-103
+              auto *PSBR = dyn_cast<PathSensitiveBugReport>(&BR);
+              if (PSBR) {
+                if (PSBR->isInteresting(Field)) {
+                  PSBR->markInteresting(Cont);
+                }
+              }
+              return "";
----------------
Probably a flattened version would be more readable.
```lang=c++
auto *PSBR = dyn_cast<PathSensitiveBugReport>(&BR);
if (PSBR && PSBR->isInteresting(Field))
  PSBR->markInteresting(Cont);
return "";
```


================
Comment at: clang/test/Analysis/container-modeling.cpp:211
 
-  V2.push_back(n); // expected-note{{Container 'V2' extended to the right by 1 position}} FIXME: This note should not appear since `V2` is not affected in the "bug"
+  V2.push_back(n); // no note expected
 
----------------
I'm not sure about the convention about using //dashes//, but I've seen multiple time comments like this:
`no-warning` etc. Probably a simple `no-note` or something would be more conventional?


================
Comment at: clang/test/Analysis/container-modeling.cpp:226
 
-  V1.push_back(n); // expected-note{{Container 'V1' extended to the right by 1 position}}
-                   // expected-note at -1{{Container 'V1' extended to the right by 1 position}} FIXME: This should appear only once since there is only one "bug" where `V1` is affected
+  V1.push_back(n); // expected-note{{Container 'V1' extended to the right by 1 position}} -- Only once!
 
----------------
This line looks quite crowded, can't we use `expected-note at -1` here just like it was used previously?
The `only once` comment could be in a separate line as well, just to fit nicely.


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

https://reviews.llvm.org/D75514





More information about the cfe-commits mailing list