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

Burninate IDataPack #864

Merged
merged 2 commits into from Aug 13, 2018
Merged

Burninate IDataPack #864

merged 2 commits into from Aug 13, 2018

Conversation

@asherkin
Copy link
Member

@asherkin asherkin commented Jul 31, 2018

This doesn't break any extensions NOT using IDataPack, and we do not know of any that are.

  • The extension storage utility of this interface has been broken for the last 9 months, with ISourceMod::CreateDataPack being disabled.
  • The plugin interop utility of this interface (its stated purpose) has been broken for the last 11+ years, with ISourceMod::GetDataPackHandleType being disabled.

I imagine it only survived the first cleanup 11 years ago because CSS:DM was using it internally, which it has now been migrated away from.

Compiled all the included extensions without changes (API compat), and loaded extensions built pre-change without issue (ABI compat).

This doesn't break any extensions NOT using IDataPack, and we do not know of any that are.

* The extension storage utility of this interface has been broken for the last 9 months, with ISourceMod::CreateDataPack being disabled.
* The plugin interop utility of this interface (its stated purpose) has been broken for the last 11+ years, with ISourceMod::GetDataPackHandleType being disabled.

I imagine it only survived the first cleanup 11 years ago because CSS:DM was using it internally, which it has now been migrated away from.

Compiled all the included extensions without changes (API compat), and loaded extensions build pre-change without issue (ABI compat).
@peace-maker
Copy link
Member

@peace-maker peace-maker commented Jul 31, 2018

You could copy the documentation from IDataPack.h to the CDataPack header.

Copy link
Member

@Headline Headline left a comment

If we do toss IDataPack into the incinerator, this looks good. One question inline

@@ -35,7 +35,7 @@ namespace SourceMod {

// Add 1 to the RHS of this expression to bump the intercom file
// This is to prevent mismatching core/logic binaries
static const uint32_t SM_LOGIC_MAGIC = 0x0F47C0DE - 56;
static const uint32_t SM_LOGIC_MAGIC = 0x0F47C0DE - 57;
Copy link
Member

@Headline Headline Jul 31, 2018

Choose a reason for hiding this comment

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

If we fwd-decl'd IDataPack in the SourceMod namespace and didn't change the iface declarations of CreateDataPack and FreeDataPack in sourcemod.h, could we prevent bumping this?

Copy link
Member Author

@asherkin asherkin Jul 31, 2018

Choose a reason for hiding this comment

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

The bump here is because I've removed the functions from the bridge (which were only being used by ISourceMod), we could just leave them in the bridge (even just as null ptrs) to avoid bumping this, but logic bridge iface bumps are not a problem, it is just to prevent against accidents.

Copy link
Member

@Headline Headline Jul 31, 2018

Choose a reason for hiding this comment

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

ah; makes sense. 👍

@dvander
Copy link
Member

@dvander dvander commented Jul 31, 2018

👍

@asherkin asherkin merged commit ba8b42e into master Aug 13, 2018
2 checks passed
@asherkin asherkin deleted the no-more-dp branch Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants