-
Notifications
You must be signed in to change notification settings - Fork 62
ceed - replace usage of ceed->op_fallback_parent with ceed->parent
#1856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
jeremylt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent and fallback parent are different logically though
|
Are they though? Both of them refer to the |
|
They are different logically. If we need both is a different question |
|
Okay. I think that in a fallback |
|
So perhaps they are logically distinct, but abstractly I think they serve the same role |
|
Thinking it over, I'm sold. Ok, so other question here then - creating a Ceed object is cheap, should we just create the fallback Ceed itself instead of creating the string? (Then we could set the fallback Ceed and delegate Ceed and the same Ceed in some cases so would need to make sure the logic plays out correctly) |
|
Hmm that seems reasonable, since then parent would be the same anyway. To me, fallback is essentially a weak delegate. In fact, I feel like we could probably replace most instances of I understand if that's mixing different logical components too much though. |
|
I am not sure that the delegate will work. A delegate is designed to fully replace the full object functionally. |
|
Okay, I just updated to make the operator fallback interface more like the delegate interface, lmk what you think |
jeremylt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure all the JiT paths are copied over as needed
|
Really, Ratel running correctly with this branch for GPU would verify the changes |
|
The JiT paths no longer need to be copied at all, since, the JiT paths come from the parent. I've been running Ratel with this all day, so I think it's good! |
|
Cool, my only quibble now is that the developer documentation needs to be updated to reflect this https://github.com/CEED/libCEED/blob/main/doc%2Fsphinx%2Fsource%2FlibCEEDdev.md |
jeremylt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this update
The
ceed->op_fallback_parentmember is redundant and creates potential data mismatches between a givenCeedcontext and its children. This removes the need to manually copy over JiT source roots and defines from the parent to fallbackCeedand prevents possible footguns when using theCeed*Parent*interfaces.