[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
steakhal added inline comments.
Comment at: clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp:54
+ for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+delete Consumer;
}
steakhal wrote:
> xazax.hun wrote:
> > Hm, I wonder whether we actually want to use `unique_ptr`s to make the
> > ownership explicit. Feel free to leave as is in this PR.
> The ownership model of these consumers is so messed up.
> It's not that simple to fix. I was bitten by dangling consumers/graphs so
> many times now only counting **last** year.
> So, yea. Maybe one day.
Fixed by https://reviews.llvm.org/D154478
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154325/new/
https://reviews.llvm.org/D154325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
xazax.hun accepted this revision.
xazax.hun added a comment.
I love it, I think we can land this as is. If there are further comments, we
can address those in follow-up PRs.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1190
CXXRecordDecl::field_iterator CurField = LE->getLambdaClass()->field_begin();
- for (LambdaExpr::const_capture_init_iterator i = LE->capture_init_begin(),
- e = LE->capture_init_end();
- i != e; ++i, ++CurField, ++Idx) {
+ for (auto i = LE->capture_init_begin(), e = LE->capture_init_end(); i != e;
+ ++i, ++CurField, ++Idx) {
steakhal wrote:
> steakhal wrote:
> > xazax.hun wrote:
> > > Ha about using `capture_inits`?
> > I would count this as a "fancy" iteration as we increment/advance more
> > logical sequences together.
> I had a better idea and llvm::zip all these together.
> Check it out, and tell me if you like it more ;)
> I certainly do.
Nice!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154325/new/
https://reviews.llvm.org/D154325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
steakhal added a comment.
Fixed remarks, cleaned up residual includes, rebased.
Should be read to land.
Final thoughts?
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:501
public:
+const iterator &operator*() const { return *this; }
+
steakhal wrote:
> xazax.hun wrote:
> > Is this just a dummy to make sure this satisfies some concepts? I think I
> > comment might be helpful in that case, people might find a noop dereference
> > operator confusing. Same for some other places.
> Yes. I'll add a comment.
> This falls into the "weird" iterators category.
Added comments to both places.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:749-750
referenced_vars_iterator referenced_vars_begin() const;
referenced_vars_iterator referenced_vars_end() const;
+ llvm::iterator_range referenced_vars() const;
steakhal wrote:
> xazax.hun wrote:
> > Do we need these with the addition of `referenced_vars`?
> I no longer remember where, but yes. But for sure I went through these to see
> if I could remove them.
Now I remember.
The `referenced_vars` is implemented as
`llvm::make_range(referenced_vars_begin(), referenced_vars_end())`, thus I need
those functions as they do quite a bit of work if you check.
So, I kept them.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:637-638
region_iterator region_begin() const { return LiveRegionRoots.begin(); }
region_iterator region_end() const { return LiveRegionRoots.end(); }
+ llvm::iterator_range regions() const {
steakhal wrote:
> xazax.hun wrote:
> > Should we remove these?
> I'm pretty sure we need these somewhere.
> One prominent place is where we dump these into JSONs. We do some fancy
> iteration there. I'm not sure that is the reason for this particular case.
> I'll give it another look to confirm.
This was just an oversight. Removed. Thanks!
Comment at: clang/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp:72
-for (auto B=CondS->symbol_begin(), E=CondS->symbol_end(); B != E; ++B) {
- const SymbolRef Antecedent = *B;
+for (SymbolRef Antecedent : CondS->symbols()) {
State = addImplication(Antecedent, State, true);
Oh, I didn't know this is a word :D
Comment at: clang/lib/StaticAnalyzer/Core/Environment.cpp:29
#include "llvm/ADT/ImmutableMap.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallPtrSet.h"
Removed this extra include.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:173-177
+} else {
// The left-hand side may bind to a different value then the
// computation type.
LHSVal = svalBuilder.evalCast(Result, LTy, CTy);
+}
steakhal wrote:
> xazax.hun wrote:
> > Might be an issue with Phab, but indent looks strange to me here.
> Thanks for catching this. Indeed. I'll have a look. Probably I messed
> something up.
Clang-format went crazy, and I'm using clang-format on save, thus messed this
up.
Fixed up manually. Should be good now.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1190
CXXRecordDecl::field_iterator CurField = LE->getLambdaClass()->field_begin();
- for (LambdaExpr::const_capture_init_iterator i = LE->capture_init_begin(),
- e = LE->capture_init_end();
- i != e; ++i, ++CurField, ++Idx) {
+ for (auto i = LE->capture_init_begin(), e = LE->capture_init_end(); i != e;
+ ++i, ++CurField, ++Idx) {
steakhal wrote:
> xazax.hun wrote:
> > Ha about using `capture_inits`?
> I would count this as a "fancy" iteration as we increment/advance more
> logical sequences together.
I had a better idea and llvm::zip all these together.
Check it out, and tell me if you like it more ;)
I certainly do.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154325/new/
https://reviews.llvm.org/D154325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
steakhal planned changes to this revision.
steakhal marked an inline comment as done.
steakhal added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:501
public:
+const iterator &operator*() const { return *this; }
+
xazax.hun wrote:
> Is this just a dummy to make sure this satisfies some concepts? I think I
> comment might be helpful in that case, people might find a noop dereference
> operator confusing. Same for some other places.
Yes. I'll add a comment.
This falls into the "weird" iterators category.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:749-750
referenced_vars_iterator referenced_vars_begin() const;
referenced_vars_iterator referenced_vars_end() const;
+ llvm::iterator_range referenced_vars() const;
xazax.hun wrote:
> Do we need these with the addition of `referenced_vars`?
I no longer remember where, but yes. But for sure I went through these to see
if I could remove them.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:637-638
region_iterator region_begin() const { return LiveRegionRoots.begin(); }
region_iterator region_end() const { return LiveRegionRoots.end(); }
+ llvm::iterator_range regions() const {
xazax.hun wrote:
> Should we remove these?
I'm pretty sure we need these somewhere.
One prominent place is where we dump these into JSONs. We do some fancy
iteration there. I'm not sure that is the reason for this particular case.
I'll give it another look to confirm.
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp:194-195
- for (CallExpr::const_arg_iterator ai = i->AllocCall->arg_begin(),
- ae = i->AllocCall->arg_end(); ai != ae; ++ai) {
-if (!(*ai)->getType()->isIntegralOrUnscopedEnumerationType())
+ for (const Expr *Arg : make_range(CallRec.AllocCall->arg_begin(),
+CallRec.AllocCall->arg_end())) {
+if (!Arg->getType()->isIntegralOrUnscopedEnumerationType())
xazax.hun wrote:
>
Makes sense.
Comment at: clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp:54
+ for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+delete Consumer;
}
xazax.hun wrote:
> Hm, I wonder whether we actually want to use `unique_ptr`s to make the
> ownership explicit. Feel free to leave as is in this PR.
The ownership model of these consumers is so messed up.
It's not that simple to fix. I was bitten by dangling consumers/graphs so many
times now only counting **last** year.
So, yea. Maybe one day.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:173-177
+} else {
// The left-hand side may bind to a different value then the
// computation type.
LHSVal = svalBuilder.evalCast(Result, LTy, CTy);
+}
xazax.hun wrote:
> Might be an issue with Phab, but indent looks strange to me here.
Thanks for catching this. Indeed. I'll have a look. Probably I messed something
up.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1190
CXXRecordDecl::field_iterator CurField = LE->getLambdaClass()->field_begin();
- for (LambdaExpr::const_capture_init_iterator i = LE->capture_init_begin(),
- e = LE->capture_init_end();
- i != e; ++i, ++CurField, ++Idx) {
+ for (auto i = LE->capture_init_begin(), e = LE->capture_init_end(); i != e;
+ ++i, ++CurField, ++Idx) {
xazax.hun wrote:
> Ha about using `capture_inits`?
I would count this as a "fancy" iteration as we increment/advance more logical
sequences together.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154325/new/
https://reviews.llvm.org/D154325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
donat.nagy added a comment. I don't have concrete remarks, but I would like to note that I'm happy to see this cleanup :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154325/new/ https://reviews.llvm.org/D154325 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
xazax.hun accepted this revision.
xazax.hun added inline comments.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CoreEngine.h:501
public:
+const iterator &operator*() const { return *this; }
+
Is this just a dummy to make sure this satisfies some concepts? I think I
comment might be helpful in that case, people might find a noop dereference
operator confusing. Same for some other places.
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:749-750
referenced_vars_iterator referenced_vars_begin() const;
referenced_vars_iterator referenced_vars_end() const;
+ llvm::iterator_range referenced_vars() const;
Do we need these with the addition of `referenced_vars`?
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:637-638
region_iterator region_begin() const { return LiveRegionRoots.begin(); }
region_iterator region_end() const { return LiveRegionRoots.end(); }
+ llvm::iterator_range regions() const {
Should we remove these?
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocSizeofChecker.cpp:194-195
- for (CallExpr::const_arg_iterator ai = i->AllocCall->arg_begin(),
- ae = i->AllocCall->arg_end(); ai != ae; ++ai) {
-if (!(*ai)->getType()->isIntegralOrUnscopedEnumerationType())
+ for (const Expr *Arg : make_range(CallRec.AllocCall->arg_begin(),
+CallRec.AllocCall->arg_end())) {
+if (!Arg->getType()->isIntegralOrUnscopedEnumerationType())
Comment at: clang/lib/StaticAnalyzer/Core/AnalysisManager.cpp:54
+ for (PathDiagnosticConsumer *Consumer : PathConsumers) {
+delete Consumer;
}
Hm, I wonder whether we actually want to use `unique_ptr`s to make the
ownership explicit. Feel free to leave as is in this PR.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:173-177
+} else {
// The left-hand side may bind to a different value then the
// computation type.
LHSVal = svalBuilder.evalCast(Result, LTy, CTy);
+}
Might be an issue with Phab, but indent looks strange to me here.
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:1190
CXXRecordDecl::field_iterator CurField = LE->getLambdaClass()->field_begin();
- for (LambdaExpr::const_capture_init_iterator i = LE->capture_init_begin(),
- e = LE->capture_init_end();
- i != e; ++i, ++CurField, ++Idx) {
+ for (auto i = LE->capture_init_begin(), e = LE->capture_init_end(); i != e;
+ ++i, ++CurField, ++Idx) {
Ha about using `capture_inits`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154325/new/
https://reviews.llvm.org/D154325
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
steakhal added a comment. The transformations were done by hand. So I'd encourage you all to have a look and find places where it breaks. Also, it might have personal biases about the style I settled with. Feel free to challenge. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154325/new/ https://reviews.llvm.org/D154325 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. But this renders much of my C++98 knowledge obsolete!11 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154325/new/ https://reviews.llvm.org/D154325 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer
steakhal added a comment. There remained a few more cases using raw-for loops, but those were slightly more tricky to uplift as they either use weird iterators or employ some fancy look-ahead/behind or iterate in parallel over different sequences etc. I'll also make a measurement to confirm it doesn't change any reports. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154325/new/ https://reviews.llvm.org/D154325 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
