ASDenysPetrov marked 4 inline comments as done.
ASDenysPetrov added a comment.
Thank you for the feedback. I'll update the patch soon.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:261-262
+ // remove adjacent ranges
+ newRanges = F.remove(newRanges, *iter1);
+ newRanges = F.remove(newRanges, *iter2);
+ // add united range
----------------
NoQ wrote:
> I don't remember iterator invalidation rules for these containers and it
> gives me anxiety. Given that these containers are immutable, i wouldn't be
> surprised if iterators are indeed never invalidated (just keep pointing to
> the old container) but it definitely deserves a comment.
Thanks for the notice. I'm not sure about iterator invalidation, so I'll just
replace `*iter2` with `*newRanges.begin()`.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:27-28
+ TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+ const std::initializer_list<T> &originalList,
+ const std::initializer_list<T> &expectedList)
+ : original(createRangeSetFromList(BVF, F, originalList)),
----------------
NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > AFAIK since `std::initializer_list` is just two pointers we should take
> > > it by value.
> > I'm not sure about //should//, since initializer_list is a container and
> > it'd be consistent if we handle it like any other compound object despite
> > its implementation (IMO). But I can change it if you wish.
> > `initializer_list` is a container
>
> No, it's a //view// :)
> No, it's a view :)
Yes, you are right. OK, I'll change it to value argument.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51
+ // init block
+ DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+ FileSystemOptions FileSystemOpts;
----------------
NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > Generally, `new expressions` are a code smell. We should use something
> > > like an `std::make_unique` to prevent memory leaks on exceptions.
> > > Though, I'm not sure if there is a similar function for
> > > `llvm::IntrusiveRefCntPtr<T>`s.
> > I'll make it more safe.
> I'd rather create ASTContext through tooling, like in other unittests.
I'll look into other tests and try to change to ASTContext.
================
Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:113-120
+ // c.original.print(llvm::dbgs());
+ // llvm::dbgs() << " => ";
+ // c.expected.print(llvm::dbgs());
+ // llvm::dbgs() << " => ";
+ // negatedFromOriginal.print(llvm::dbgs());
+ // llvm::dbgs() << " => ";
+ // negatedBackward.print(llvm::dbgs());
----------------
NoQ wrote:
> ASDenysPetrov wrote:
> > steakhal wrote:
> > > Should we keep this?
> > I'm not sure, but I'd better keep it, because it is useful for debugging.
> No-no, we don't do that here >.>
>
> If you constructed an awesome debugging facility and you want to save it for
> future generations, i'd recommend turning it into an actual facility, not
> just comments. Like, maybe, an unused function that can be invoked for
> debugging, or something like that.
OK, I'll move it to function.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77802/new/
https://reviews.llvm.org/D77802
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
