[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.



Comment at: clang/test/Analysis/iterator-modelling.cpp:434
+  //expected-note@-1 0-1{{Calling 'next}}
+  //expected-note@-2 0-1{{Passing the value 1 via 2nd parameter 'n'}}
+  //expected-note@Inputs/system-header-simulator-cxx.h:814 0-1{{Iterator 'it' 
incremented by 1}}

NoQ wrote:
> baloghadamsoftware wrote:
> > Strange, that we do not get this note for `prev`.
> Mmm, why `0-1`?
Because it depends whether `next()` is inlined.


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/Analysis/iterator-modelling.cpp:434
+  //expected-note@-1 0-1{{Calling 'next}}
+  //expected-note@-2 0-1{{Passing the value 1 via 2nd parameter 'n'}}
+  //expected-note@Inputs/system-header-simulator-cxx.h:814 0-1{{Iterator 'it' 
incremented by 1}}

baloghadamsoftware wrote:
> Strange, that we do not get this note for `prev`.
Mmm, why `0-1`?


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 4 inline comments as done.
baloghadamsoftware added inline comments.



Comment at: clang/test/Analysis/iterator-modelling.cpp:434
+  //expected-note@-1 0-1{{Calling 'next}}
+  //expected-note@-2 0-1{{Passing the value 1 via 2nd parameter 'n'}}
+  //expected-note@Inputs/system-header-simulator-cxx.h:814 0-1{{Iterator 'it' 
incremented by 1}}

Strange, that we do not get this note for `prev`.


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.
Herald added a subscriber: ASDenysPetrov.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:489-490
+StringRef ChangeText =
+  ((Op == OO_Plus || Op == OO_PlusEqual) != (ChangeVal <= 0)) ?
+  "incremented" : "decremented";
+const NoteTag *ChangeTag = getChangeTag(C, ChangeText, ItE,

baloghadamsoftware wrote:
> NoQ wrote:
> > NoQ wrote:
> > > Can we assert out the `ChangeVal == 0` case or make a better message for 
> > > it (`"unchanged"` or something like that)?
> > Another important thing to do here is to track the RHS value via 
> > `trackExpressionValue()`. I.e., why do we think that iterator is 
> > incremented by 1 and not by 2?
> I agree, that is important, but where should I call it? This is the modeling 
> checker, which models the increments and the decrements, but 
> `trackExpressionValue()` can only be invoked for bug reports. Should I put it 
> into the `NoteTag` lambda?
Yup, it should be possible to invoke `trackExpressionValue()`, and generally 
attach more visitors, inside a `NoteTag`.


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.
Herald added a subscriber: DenisDvlp.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:489-490
+StringRef ChangeText =
+  ((Op == OO_Plus || Op == OO_PlusEqual) != (ChangeVal <= 0)) ?
+  "incremented" : "decremented";
+const NoteTag *ChangeTag = getChangeTag(C, ChangeText, ItE,

NoQ wrote:
> NoQ wrote:
> > Can we assert out the `ChangeVal == 0` case or make a better message for it 
> > (`"unchanged"` or something like that)?
> Another important thing to do here is to track the RHS value via 
> `trackExpressionValue()`. I.e., why do we think that iterator is incremented 
> by 1 and not by 2?
I agree, that is important, but where should I call it? This is the modeling 
checker, which models the increments and the decrements, but 
`trackExpressionValue()` can only be invoked for bug reports. Should I put it 
into the `NoteTag` lambda?


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:489-490
+StringRef ChangeText =
+  ((Op == OO_Plus || Op == OO_PlusEqual) != (ChangeVal <= 0)) ?
+  "incremented" : "decremented";
+const NoteTag *ChangeTag = getChangeTag(C, ChangeText, ItE,

NoQ wrote:
> Can we assert out the `ChangeVal == 0` case or make a better message for it 
> (`"unchanged"` or something like that)?
Another important thing to do here is to track the RHS value via 
`trackExpressionValue()`. I.e., why do we think that iterator is incremented by 
1 and not by 2?


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:489-490
+StringRef ChangeText =
+  ((Op == OO_Plus || Op == OO_PlusEqual) != (ChangeVal <= 0)) ?
+  "incremented" : "decremented";
+const NoteTag *ChangeTag = getChangeTag(C, ChangeText, ItE,

Can we assert out the `ChangeVal == 0` case or make a better message for it 
(`"unchanged"` or something like that)?


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 4 inline comments as done.
baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:519-520
+const NoteTag *IteratorModeling::getChangeTag(CheckerContext , StringRef 
Text,
+  const Expr *ItE, SVal It1,
+  int64_t Amount, SVal It2) const {
+  StringRef Name;

Szelethus wrote:
> Szelethus wrote:
> > Are `It1` and `It2` used? Why do we default the latter to `UndefinedVal`?
> Ah, okay, we use it in a followup patch. Still, we shouldn't use 
> `UndefinedVal`, as discussed in D75514.
I completely removed them from this patch.


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:519-520
+const NoteTag *IteratorModeling::getChangeTag(CheckerContext , StringRef 
Text,
+  const Expr *ItE, SVal It1,
+  int64_t Amount, SVal It2) const {
+  StringRef Name;

Szelethus wrote:
> Are `It1` and `It2` used? Why do we default the latter to `UndefinedVal`?
Ah, okay, we use it in a followup patch. Still, we shouldn't use 
`UndefinedVal`, as discussed in D75514.


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

A few nits, otherwise the patch is great!




Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:519-520
+const NoteTag *IteratorModeling::getChangeTag(CheckerContext , StringRef 
Text,
+  const Expr *ItE, SVal It1,
+  int64_t Amount, SVal It2) const {
+  StringRef Name;

Are `It1` and `It2` used? Why do we default the latter to `UndefinedVal`?



Comment at: clang/test/Analysis/iterator-modelling.cpp:1-5
+// RUN: %clang_analyze_cc1 -std=c++11 
-analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection
 -analyzer-config aggressive-binary-operation-simplification=true 
-analyzer-config c++-container-inlining=false -analyzer-output=text %s -verify
 
-// RUN: %clang_analyze_cc1 -std=c++11 
-analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection
 -analyzer-config aggressive-binary-operation-simplification=true 
-analyzer-config c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 
-analyzer-checker=core,cplusplus,debug.DebugIteratorModeling,debug.ExprInspection
 -analyzer-config aggressive-binary-operation-simplification=true 
-analyzer-config c++-container-inlining=true -DINLINE=1 -analyzer-output=text 
%s -verify
 
 // RUN: %clang_analyze_cc1 -std=c++11 
-analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorModeling,debug.ExprInspection
 -analyzer-config aggressive-binary-operation-simplification=true %s 2>&1 | 
FileCheck %s

Can we prettify this? :)



Comment at: clang/test/Analysis/iterator-range.cpp:1-2
-// RUN: %clang_analyze_cc1 -std=c++11 
-analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config 
aggressive-binary-operation-simplification=true -analyzer-config 
c++-container-inlining=false %s -verify
-// RUN: %clang_analyze_cc1 -std=c++11 
-analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config 
aggressive-binary-operation-simplification=true -analyzer-config 
c++-container-inlining=true -DINLINE=1 %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 
-analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config 
aggressive-binary-operation-simplification=true -analyzer-config 
c++-container-inlining=false -analyzer-output=text %s -verify
+// RUN: %clang_analyze_cc1 -std=c++11 
-analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config 
aggressive-binary-operation-simplification=true -analyzer-config 
c++-container-inlining=true -DINLINE=1 -analyzer-output=text %s -verify
 

And this?


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

See my related comment here: D73720#1901453 
.


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

https://reviews.llvm.org/D74541



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits