vsavchenko added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:226
+ llvm::make_range(BaseList.begin(), BaseList.end()),
+ [](const auto &I) -> bool { return I.second; });
+
----------------
RedDocMD wrote:
> vsavchenko wrote:
> > I think it's all a bit too complicated now.
> >
> > Right now you have two nested loops:
> > 1. over `PathRange`
> > 2. over `BaseList` (in `find_if`)
> >
> > It won't be a problem to put them in another order, so when you make a
> > `filter_range`, you can have `llvm::find_if` which will iterate over
> > `PathRange` and captures the given `BaseList` element. So, yes nested
> > lambdas.
> >
> > Here is a sketch of the solution:
> > ```
> > auto BaseListFilteredRange = llvm::make_filter_range(
> > BaseList, // you don't need to make_range here, BaseList IS a range
> > [&PathRange](const CXXBaseSpecifier *Base) {
> > return llvm::find_if(PathRange, [Base](const auto &It) {
> > return Base->getType() == It.first->getType();
> > }) == PathRange.end();
> > });
> > ```
> >
> > Aaaand while I was writing that, it got me thinking that we don't need
> > `BaseList` in the first place:
> > ```
> > for (const CXXBaseSpecifier *Base : PathList)
> > if (llvm::none_of(PathRange,
> > [Base](const auto &It) { return Base->getType() ==
> > It.first->getType(); })
> > PathList = CXXBaseListFactory.add(Base, PathList);
> > ```
> Hmm, I really should brush up on my functional programming and think more in
> terms of STL/LLVM algorithms. Thank you for pointing out this solution,
> @vsavchenko!
>
> One thing I noticed though, the shortened logic that you suggested removes
> all matching instances from PathList, while my intention was to remove only
> the first one of every kind. If there are no repetitions, it is the same.
> And, normally, repetitions should not be present in the PathList retrieved
> from PTM. It is a bug elsewhere if that happens. So should I add an assert
> for that here?
Yes, that's true.
Assertions will usually make intensions of the original author more clear for
future readers.
Plus, I think it can be also good to add a comment (something similar to your
summary for this change) to describe what is going on here and WHY it is done.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:531-532
+ svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
llvm::make_range<CastExpr::path_const_iterator>(
- CastE->path_begin(), CastE->path_end()), *PTMSV));
+ CastE->path_begin(), CastE->path_end()),
+ *PTMSV, CastE->getCastKind()));
----------------
BTW, it looks like this can be replaced by `CastE->path()`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95877/new/
https://reviews.llvm.org/D95877
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
