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

[#942] GDBControl discards ILaunchConfiguration in its constructor #989

Closed
wants to merge 1 commit into from

Conversation

ruspl-afed
Copy link
Member

add protected final field to be available for successors
adding final accessor method may break Embedded CDT

Fixes #942

…structor

add protected final field to be available for successors
adding final accessor method may break Embedded CDT
@ruspl-afed
Copy link
Member Author

Interesting, why adding a field to a class considered as a major change

Error:  [API ERROR] File GDBControl.java at line 117: The field org.eclipse.cdt.dsf.gdb.service.command.GDBControl.config has been added to a class (location: /work/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/GDBControl.java)
Error:  [API ERROR] File MANIFEST.MF at line 6: The major version should be incremented in version 7.2.0, since API breakage occurred since version 7.1.300 (location: /work/dsf-gdb/org.eclipse.cdt.dsf.gdb/META-INF/MANIFEST.MF)

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.

Interesting, why adding a field to a class considered as a major change

Error:  [API ERROR] File GDBControl.java at line 117: The field org.eclipse.cdt.dsf.gdb.service.command.GDBControl.config has been added to a class (location: /work/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/GDBControl.java)
Error:  [API ERROR] File MANIFEST.MF at line 6: The major version should be incremented in version 7.2.0, since API breakage occurred since version 7.1.300 (location: /work/dsf-gdb/org.eclipse.cdt.dsf.gdb/META-INF/MANIFEST.MF)

Please see https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/Evolving-Java-based-APIs-2.md#:~:text=Adding%20an%20API%20field%20to%20an%20API%20class%20that%20is%20extended%20by%20Clients%20is%20dangerous%20in%20two%20respects%3A

So this change requires a major version bump as it is possibly breaking change. That is ok in principle for CDT 12 as that is a major version. But you have to decide whether CDT API consumers are likely to be affected negatively more or less than how you benefit from this change.

I am +0 on this change as on a quick examination of the open source and closed source extensions of GDBControl I don't actually see anyone who has their own field called config

@ruspl-afed
Copy link
Member Author

So this change requires a major version bump as it is possibly breaking change

Thank you for review @jonahgraham
Now I see. I don't think this small improvement is worth the breaking changes. FYI @jld01
Let's close this PR

@ruspl-afed ruspl-afed closed this Jan 6, 2025
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.

GDBControl discards ILaunchConfiguration parameter in its constructor
2 participants