[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96976#2585498 <https://reviews.llvm.org/D96976#2585498>, @RedDocMD wrote:

> @steakhal, could you please review this?

The longer I stare at it the more questions I have. However, I don't know how member pointers work in the analyzer and I'm not really in the mood to dive deep into that territory.

Could you elaborate on what approach did you choose and more importantly, why that approach?
Why do we need a graph search here?
What cases do you try to resolve? I mean, there is a single test-case, which has reinterpret casts back and forth (literally), and it's not immediately clear to me why are you doing those gymnastics.
Maybe worth creating other test-cases as well. Probably covering multiple inheritance or virtual inheritance?

Your code has shared pointers, which is interesting, but I would favor unique pointers unless you can defend this use-case.
Also, in general, we prefer algorithms to raw for or while loops if they are semantically equivalent.
Besides all of these, I can see a few copy-pasted lines here and there. I'm not saying that it's bad, I'm just highlighting this fact.

I can not confirm the implementation and accept the patch.
Improving the summary, helping reviewers with helpful notes here and there could give the boost you need to get this reviewed.
BTW, the review process takes ages in the analyzer community. We should also improve on that ofc, but the first step is always done by you (and here I mean not specifically you but we all).



================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:554-555
+        }
+        // Explicitly proceed with default handler for this case cascade.
+        state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+        continue;
----------------
I know that you just copy-pasted this, but what is this xD


================
Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:5
 
 // TODO: The following test will work properly once reinterpret_cast on pointer-to-member is handled properly
 namespace testReinterpretCasting {
----------------
I assume this is resolved by now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976