-
Notifications
You must be signed in to change notification settings - Fork 199
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
Remove deprecated binary parsers and supporting code #135
Remove deprecated binary parsers and supporting code #135
Conversation
e84257d
to
996acf6
Compare
You also have to update the plugin.xml too, right? https://github.com/eclipse-cdt/cdt/blob/main/core/org.eclipse.cdt.core/plugin.xml#L75 |
Yes. It was rather frustrating that this wasn't caught automatically by deleting the class, it will be with: |
996acf6
to
7cc2284
Compare
That turned out to be more to unravel than I expected. This change now needs #137 merging too. |
Nothing bad happens now that #137 is merged. However if a project has unknown binary parsers in it, opening the Binary Parsers tab and saving the results will remove the unknown parsers from the .cproject file. |
It looks like some removed IDs are still used in this plugin.xml: cdt/build/org.eclipse.cdt.autotools.core/plugin.xml Lines 396 to 397 in b37e9c3
Should org.eclipse.cdt.core.PE;org.eclipse.cdt.core.Cygwin_PE be changed here to org.eclipse.cdt.core.PE64;org.eclipse.cdt.core.Cygwin_PE64 ? Edit: It definitely should change. I can still create new projects with deprecated parsers!!! :-o |
Not sure what will happen with a lot of existing .cproject files if user will not open "Binary Parsers" tab @jonahgraham |
Argh. How long is this piece of string.... |
Thanks for the reviews - I will address all of it before merging. The fact that projects in CDT 10.7 were still being created with the long deprecated parser ids lead me to want to use a different solution in the IDs. New PS coming soon. |
Turns out there is no schema for |
7cc2284
to
1e718c8
Compare
The unknown binary parser IDs are ignored. If no suitable binary parser exists in the project then binaries won't be identified at all.
I have now pushed my alternate solution - this was what I originally considered, but I liked the idea of just removing the old stuff and progressing forward. What I have done is left the old IDs in place, but pointing at the new classes. The old IDs are private, so they don't appear in the GUI as an option for users to select anymore. WDYT? |
So, it will continue to understand existing (old) identifiers, but will use *64 code. And the existing projects will not be affected at all, correct? |
Correct. We'll just live with the old identifiers forever. In hindsight this changing which class is used for the identifier could have been done back in CDT 6.9 instead of creating new identifiers at all. |
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.
I support the solution that does not impact existing user data
The one failed test is what is tracked/fixed by #128 |
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.
LGTM
This extension has been in existence since very early days of CDT, but it never(?) had a schema, so the improvements done in eclipse-cdt#136 will now show errors in use of this extension.
These binary parsers have been slated for deleting for a while and are replaced with 64-bit compatible versions. Some methods still refereneced the 32-bit variants and have been updated to the fully functioning 64-bit variant. The older parser IDs are preserved (forever?) so that old projects can be opened without needing to do anything. The IDs now point at the new implementations. See also Bug 562495
1e718c8
to
ec83784
Compare
These binary parsers have been slated for deleting for a while and are replaced with 64-bit compatible
versions.
Some methods still refereneced the 32-bit variants and have been updated to the fully functioning
64-bit variant.
See also Bug 562495
TODO before merging