Skip to content
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

[#952] Provide access for launch attributes map field #991

Closed
wants to merge 1 commit into from

Conversation

ruspl-afed
Copy link
Member

  • make fAttributes protected and remove code duplications

* make `fAttributes` protected and remove code duplications
Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

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

I am +0 on this too (see #989) even though I see some extenders do have their own field. This basically causes new warnings in extenders who have their own fAttributes already.

@jonahgraham
Copy link
Member

The 8 errors 60 fail is known failures, see #816

@ruspl-afed
Copy link
Member Author

Thank you for review @jonahgraham
It looks like it is better for all to leave the current implementation [dying] as it is, because any change that looks like improvement (for example, because it reduces the code duplication) causes potential issues.
I tried to integrate back changes I did for debug scenario on our product that allowed me to "heal" DSF code via removing duplications (thousands of LOC) for both CDT and EmbedCDT. I tried to select some of them (the most laconic and simple) and had no luck. Perhaps, this is clear sign that CDT DSF is mature enough to change anything and it is easier to write an alternative [with less cumbersome and more maintainable code] than gradually improve the existing one.

I'm fine to close this PR as well. But before doing so, I would like to learn how do you see the future of DSF @jonahgraham @jld01

@jonahgraham
Copy link
Member

I would like to learn how do you see the future of DSF

I see it similar to many other parts of CDT + Eclipse ecosystem. An absolutely critical part of the CDT offerings, under-maintained but generally very good.

@ruspl-afed
Copy link
Member Author

Thank you for the guidance @jonahgraham, I've learned that I need to be much more careful before suggesting changes to the CDT codebase.
I will close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants