Skip to content

DependencyGraph: use ConfigObject*, not Object*#10264

Merged
Al2Klimov merged 1 commit intomasterfrom
DependencyGraph-ConfigObject
Dec 18, 2024
Merged

DependencyGraph: use ConfigObject*, not Object*#10264
Al2Klimov merged 1 commit intomasterfrom
DependencyGraph-ConfigObject

Conversation

@Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Dec 4, 2024

This saves dynamic_cast<ConfigObject*> + if() on every item of GetChildren().

TODO

@Al2Klimov Al2Klimov added the core/quality Improve code, libraries, algorithms, inline docs label Dec 4, 2024
@Al2Klimov Al2Klimov self-assigned this Dec 4, 2024
@cla-bot cla-bot bot added the cla/signed label Dec 4, 2024
@Al2Klimov Al2Klimov mentioned this pull request Dec 4, 2024
2 tasks
@Al2Klimov Al2Klimov force-pushed the DependencyGraph-ConfigObject branch from 5547c12 to af064c8 Compare December 4, 2024 11:37
@Al2Klimov Al2Klimov requested a review from yhabteab December 12, 2024 10:40
Copy link
Member Author

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • To be clear, this is not a requirement for #10000. But as you re-built the DG there and will re-work that due to #10263 anyway, I think it makes sense to make the DG proper right away. Unless, of course, you don't like this PR.

@Al2Klimov Al2Klimov marked this pull request as ready for review December 12, 2024 14:13
@Al2Klimov Al2Klimov removed their assignment Dec 12, 2024
@Al2Klimov
Copy link
Member Author

Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this PR is just about replacing Object* with ConfigObject*, why did you make so many unrelated changes? For example, why did you replace std::map with std::unordered_map, knowing that this will be changed with #10000 anyway? If you want me to review this, then please drop all the unrelated changes that are not related to what the PR title advertises.

@Al2Klimov Al2Klimov force-pushed the DependencyGraph-ConfigObject branch from af064c8 to 3022d0c Compare December 16, 2024 16:00
@Al2Klimov Al2Klimov requested a review from yhabteab December 16, 2024 16:00
@Al2Klimov
Copy link
Member Author

I've removed the top commit. (af064c8)

@Al2Klimov Al2Klimov removed the request for review from julianbrost December 16, 2024 16:09
@Al2Klimov Al2Klimov force-pushed the DependencyGraph-ConfigObject branch 2 times, most recently from 6db2662 to 78c68b4 Compare December 16, 2024 18:31
@Al2Klimov Al2Klimov requested a review from yhabteab December 16, 2024 18:32
@Al2Klimov Al2Klimov requested a review from yhabteab December 17, 2024 09:00
@Al2Klimov Al2Klimov force-pushed the DependencyGraph-ConfigObject branch from 78c68b4 to 5e81b4d Compare December 17, 2024 09:33
This saves dynamic_cast<ConfigObject*> + if() on every item of GetChildren().
@Al2Klimov Al2Klimov force-pushed the DependencyGraph-ConfigObject branch from 6db2662 to 3a09cf7 Compare December 17, 2024 17:33
@Al2Klimov Al2Klimov merged commit 383773e into master Dec 18, 2024
@Al2Klimov Al2Klimov deleted the DependencyGraph-ConfigObject branch December 18, 2024 12:36
@Al2Klimov Al2Klimov added this to the 2.15.0 milestone Dec 18, 2024
@yhabteab yhabteab added the consider backporting Should be considered for inclusion in a bugfix release label Jan 13, 2025
@Al2Klimov Al2Klimov added backported Fix was included in a bugfix release and removed consider backporting Should be considered for inclusion in a bugfix release labels Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported Fix was included in a bugfix release cla/signed core/quality Improve code, libraries, algorithms, inline docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants