RedDocMD marked 2 inline comments as done.
RedDocMD added a comment.
Many thanks for you comments, @steakhal!
I will address the issues you have pointed out in this comment. To clean things
up I should perhaps add some more clarification to the summary.
> Could you elaborate on what approach did you choose and more importantly, why
> that approach?
> Why do we need a graph search here?
Pointer-to-members contain two things - a pointer to a NamedDecl to store the
field/method being pointed to and a list of CXXBaseSpeciifier. This list is
used to determine which sub-object the member lies in. This path needs to be
determined and unfortunately with `reinterpret_cast`, the AST provides no
structural assistance (unlike `static_cast`). Hence, it needs to be searched by
searching through the BaseSpecifiers of the `CXXRecordDecl`.
As for the need of graph search, it is due to multiple-inheritance. For a given
class, it may have two or more bases and we need to follow into both of them to
find the path to the required sub-object.
Why do I use BFS? Because one branch of the (inverted) inheritance tree may be
very deep yet not contain the required class. But DFS will first exhaust it and
then go the other branch. BFS on the other hand will find the shortest path to
the correct base optimally, and this shortest path is all we need.
> 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?
I will add a test case for multiple inheritance and perhaps replace existing
tests with clearer tests. Virtual inheritance will not help since
pointer-to-member to virtual base field/method is not allowed.
There are three cases which I have handled:
1. The required class is the class we are casting to
2. The required class is higher in the class hierarchy
3. The required class is lower in the class hierarchy
4. The required class is not related to the class being cast to.
For Case 1 and 2, a valid path can be calculated and so a pointer-to-member is
created.
For Case 3 and 4, no valid path can be calculated and so it doesn't make sense
to further pursue static analysis down this path.
Note that the required class is determined by the actual field/member we are
pointing to and is not explicit in the `reinterpret_cast`.
> Your code has shared pointers, which is interesting, but I would favor unique
> pointers unless you can defend this use-case.
The reason for not using `unique_ptr` is as follows:- each `BFSNode` holds a
pointer to its parent. This cannot be a `unique_ptr` since there may be
multiple children to a `BFSNode`. This must be a `shared_ptr` as a consequence.
> Also, in general, we prefer algorithms to raw for or while loops if they are
> semantically equivalent.
I am not aware of any std/LLVM algorithm that perform BFS. If there are any, I
would be glad to learn more about them.
I am going to replace the for loops with a range-for loop. Sorry about that one!
> 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.
Yes this was a conscious choice. The other alternative perhaps is to use
fall-through, which I think would be confusing in this case since it will be
obscured by the intermediate code.
================
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;
----------------
steakhal wrote:
> I know that you just copy-pasted this, but what is this xD
> I know that you just copy-pasted this, but what is this xD
Not entirely sure, but I think it performs tasks required to handle the
pointer-to-member created or do the requisite default in the absence of a
pointer-to-member.
================
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 {
----------------
steakhal wrote:
> I assume this is resolved by now.
> I assume this is resolved by now.
Sorry, that was a lame mistake on my part.
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
