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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Thanks for taking your time and getting to the root cause of it!



================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:209
+    // thing about Clang AST.
+    std::list<const CXXBaseSpecifier *> BaseList;
+    for (const auto &I : PathList)
----------------
`std::list` is like the slowest container, and it should be avoided in almost every case.
Considering the semantics of it, you can definitely use `llvm::SmallVector`.


================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:222
+    for (const auto &PathBase : llvm::reverse(PathRange)) {
+      auto DelIt = find_first(BaseList.begin(), BaseList.end(), PathBase);
+      assert(DelIt != BaseList.end() && "PTM has insufficient base specifiers");
----------------
`llvm::find_if` with a lambda capturing `PathBase`?


================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:223
+      auto DelIt = find_first(BaseList.begin(), BaseList.end(), PathBase);
+      assert(DelIt != BaseList.end() && "PTM has insufficient base specifiers");
+      BaseList.erase(DelIt);
----------------
It's better to be more verbose in the assertions.
Additionally, I'm not sure that it is clear what it is all about because pointer-to-members do not have base specifiers.


================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:224
+      assert(DelIt != BaseList.end() && "PTM has insufficient base specifiers");
+      BaseList.erase(DelIt);
+    }
----------------
In order to avoid removals from the middle, you can use a more functional approach to collections and iterators and use `llvm::make_filter_range` and iterate over it in the loop below,


================
Comment at: clang/test/Analysis/pointer-to-member.cpp:304
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast<int Grandfather::*>(sf);
+  int Grandfather::*gpf2 = static_cast<int Grandfather::*>(static_cast<int Father::*>(sf));
----------------
What about other type of casts?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95877/new/

https://reviews.llvm.org/D95877