[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

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