Access parent from a child interpreter, and const correct gCling. #170
Conversation
Mangling with a GlobalDecl is only really necessary for constructors and destructors.
|
This also opens a pathway to move what is currently static variables in CIFactory.cpp as a property of the root-level Interpreter and enforce that all children have the same include paths as a parent. |
| @@ -142,6 +142,11 @@ namespace cling { | |||
| /// | |||
| InvocationOptions m_Opts; | |||
|
|
|||
| ///\brief Back pointer to *this* which stores parent information. | |||
There was a problem hiding this comment.
This is a confusing phrasing. Why would one need a pointer to "this"? (As oppose to a pointer to a parent that might sometime deprecates to being equal to "this". Could you rephrase and/or clarify? Thanks.
There was a problem hiding this comment.
This is necessary as putting a symbol into the JIT requires a constant location where it is available and &this is illegal.
There was a problem hiding this comment.
Indeed, after reading the whole patch, this is what I guess. Could you still rephrase the doc string to be more explicit/clear?
There was a problem hiding this comment.
Thanks. My apologies for not noticing.
| #ifdef __cplusplus | ||
| #ifndef __cplusplus | ||
|
|
||
| extern void* gCling; |
There was a problem hiding this comment.
Can it also be 'void* const' for consistency?
| /// success or an empty string on failure. | ||
| /// | ||
| std::string maybeMangleDeclName(const clang::NamedDecl* ND, | ||
| const clang::GlobalDecl* GD = nullptr); |
There was a problem hiding this comment.
Why the change in interface style (returning the string rather than modifying a pass string)? For consistency sake we should only have one style (The existing style used to be more performance but nowadays with move semantic this is no longer the case).
There was a problem hiding this comment.
That was a separate PR as it had nothing to do with this, but pulled it in.
There was a problem hiding this comment.
That was a separate PR
Was or is? Is it still intended to be changed?
There was a problem hiding this comment.
I rolled it into this one d14e9b4
There was a problem hiding this comment.
Then I am a bit confused (i.e. I am missing something). d14e9b4 seems to be in the master for a long while, while this PR seems to not include it ... what am I missing?
There was a problem hiding this comment.
Huh? Now I'm confused.
d14e9b4 is right below this comment section.
If it appears in any master it would be an interesting SHA1 collision.
lib/Interpreter/Interpreter.cpp
Outdated
| using namespace utils; | ||
| const std::string Name = Analyze::maybeMangleDeclName(gCling); | ||
| if (!Name.empty()) { | ||
| // gCling gets linked to top-most Interpreter. |
There was a problem hiding this comment.
If I read the code correctly, the code below does not implement the comment (since m_Parenting[0] is always set to 'this' rather than the parent.
In addition, I fail to see/understand the code that would prevent the symbol to be added multiple time (one per interpreter).
What am I missing?
There was a problem hiding this comment.
My mistake, this was culled from a changes that moved to thisCling which links to the child as parent can always be reached through it.
marsupial
force-pushed
the
PR/19
branch
2 times, most recently
from
8940cdc
to
5466e28
Compare
Also makes RuntimeUniverse.h includable by languages other than C++, simplifying the code injected at startup.
By storing the parent-child relationship in the Interpreter gCling can be at the link level rather than writing it's value explicitly via casting.
This solves a mangling issue (at least on OS X), so that gCling can properly be made a const pointer.
The ultimate goal of this is to move the pointer gCling to a reference of thisCling for all Interpreters including children, who can then access their parent interpreter via thisCling.parent() or the top-most instance via thisCling.ancestor().