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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
