-
Notifications
You must be signed in to change notification settings - Fork 574
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
Sort config types by their load dependencies once #10148
Conversation
8a2ee5a
to
f78108d
Compare
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.
Compared to #10003, this PR has no cycle detection (at least I believe the purpose of some of the code might be cycle detection, it's not entirely clear though: #10003 (comment)). When I intentionally introduced a dependency in the *.ti
files, nothing in mkclass stopped me from doing so and all I got was a not very helpful error message in a test (see inline comment). So some more explicit "this failed due to a cycle" error reporting/VERIFY()
might be a good idea.
f78108d
to
7c56ec3
Compare
bfcfe80
to
e019556
Compare
e019556
to
e011597
Compare
b21a41f
to
2c308dd
Compare
2c308dd
to
9442803
Compare
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.
Looks fine and should do the job, I just don't want to do a quick approve + merge right before leaving for vacation and another pair of eyes shouldn't hurt. But I'd say you can optimistically adapt #10000 already.
I'm not 100% happy regarding the needs for the test changes including the CMakeLists changes, but that's probably what happens when relying on linker/loader magic for the type registration (the alternative would be to perform an extra self check within the implementation, so that it's checked every single time, but that's also not too nice).
No more open requests for changes, see previous comment.
9442803
to
abb068e
Compare
Oh! We can't backport this to 2.13.x due to 33e609d. |
Do you mean "We can't backport this to 2.13.x without 33e609d"? |
abb068e
to
b85fc9f
Compare
Just rebased it! |
b85fc9f
to
cb4fe57
Compare
…es once to avoid complicated nested loops, iterating over the same types and checking dependencies over and over, skipping already completed ones.
cb4fe57
to
eb97676
Compare
types/instantiate | ||
types/sort_by_load_after | ||
) | ||
|
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.
This is fine. 🎩🐶☕🔥🏠
Since you're back now :) and the PR is still open, do you have any change requests? Otherwise, please give your approval as well. |
This PR is basically the same as #10003, but since @Al2Klimov is out for this week, we need to get it done. So I've cherry-picked the commits from there and added some additional commits to address the requested changes in that PR.
closes #10003