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_iterator> 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<region_iterator> 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
