[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:180
 
+bool noRepeatedElements(
+    const llvm::ImmutableList<const CXXBaseSpecifier *> &BaseSpecList) {
----------------
nit: functions represent actions, in languages verbs act the same way, so it's recommended to use verbs in functions (`hasNoRepeatedElements`)

+ `LLVM_MAYBE_UNUSED` because in the build without assertions it would be a warning otherwise.


================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:181
+bool noRepeatedElements(
+    const llvm::ImmutableList<const CXXBaseSpecifier *> &BaseSpecList) {
+  // First we need to make sure that there are no-repetitions in BaseSpecList
----------------
`ImmutableList` is one of those value types that prefers copying for clarity.


================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:184
+  llvm::SmallPtrSet<QualType, 16> BaseSpecSeen;
+  for (const auto &BaseSpec : BaseSpecList) {
+    auto BaseType = BaseSpec->getType();
----------------
LLVM Coding Style calls for very limited use of `auto`, so here and the next line would be better with actual types.


================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:184
+  llvm::SmallPtrSet<QualType, 16> BaseSpecSeen;
+  for (const auto &BaseSpec : BaseSpecList) {
+    auto BaseType = BaseSpec->getType();
----------------
vsavchenko wrote:
> LLVM Coding Style calls for very limited use of `auto`, so here and the next line would be better with actual types.
Let's also reiterate on using functional-style predicates and reimplement it in terms of `llvm::all_of` or `llvm::none_of`.


================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:232
+    auto ReducedBaseSpecList = CXXBaseListFactory.getEmptyList();
+    for (const auto &BaseSpec : BaseSpecList)
+      if (llvm::none_of(
----------------
The same thing about `auto`


================
Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:234
+      if (llvm::none_of(
+              PathRange, [&BaseSpec](const auto &I) -> auto {
+                return BaseSpec->getType() == I->getType();
----------------
No need


================
Comment at: clang/test/Analysis/pointer-to-member.cpp:314-333
+// namespace testReinterpretCasting {
+// struct Base {
+//   int field;
+// };
+//
+// struct Derived : public Base {};
+//
----------------
Uncomment it, and expect the actual current result.  This is where `TODO` will come in handy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877