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

Introduce KALEIDOSCOPE_API_VERSION #270

Merged
merged 2 commits into from Jan 29, 2018
Merged

Introduce KALEIDOSCOPE_API_VERSION #270

merged 2 commits into from Jan 29, 2018

Conversation

algernon
Copy link
Contributor

As we guarantee backwards compatibility throughout a major version, it helps if we have that version available, so plugins / sketches can check if they are compatible, and issue a helpful error if they are not. As further convenience, defining KALEIDOSCOPE_REQUIRED_API_VERSION before including Kaleidoscope.h will result in a check being done by Kaleidoscope, and an error printed if it does not match the API shipped.

As we guarantee backwards compatibility throughout a major version, it helps if
we have that version available, so plugins / sketches can check if they are
compatible, and issue a helpful error if they are not. As further convenience,
defining `KALEIDOSCOPE_REQUIRED_API_VERSION` before including `Kaleidoscope.h`
will result in a check being done by Kaleidoscope, and an error printed if it
does not match the API shipped.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
@algernon algernon added enhancement New feature or request pending labels Dec 17, 2017
@algernon algernon requested a review from obra December 17, 2017 07:19
@algernon
Copy link
Contributor Author

Intended usage:

#define KALEIDOSCOPE_REQUIRED_API_VERSION 2
#include "Kaleidoscope.h"

or

#include "Kaleidoscope.h"

#if KALEIDOSCOPE_API_VERSION != 1
#error This plugin requires Kaleidoscope version 1.x!
#endif

or

#include "Kaleidoscope.h"

// ... somewhere in the code ...

#if KALEIDOSCOPE_API_VERSION == 1
// do something supported by Kaleidoscope v1.x
#elif KALEIDOSCOPE_API_VERSION == 2
// do something supported by Kaleidoscope v2.x
#else
#error Unsupported Kaleidoscope version. Only v1.x and 2.x are supported.
#endif

@cdisselkoen
Copy link
Contributor

Would it be possible for the API mismatch error message to also state what the required and observed version numbers are? I think that would add to the helpfulness of the error message.

I will also note that since the check enforces that the required API version and provided API version exactly match, any bump of the API version number in Kaleidoscope automatically breaks all plugins. Plugins have no way to depend on, say, API version >= 2. This is probably the desired behavior - during an API version bump, all plugins have to manually inspect themselves and mark themselves compatible with the new version after making any required changes.

Is it possible for plugins to mark themselves compatible with more than one version? E.g., in @algernon's third example in his comment above, I suppose that plugin could just not define KALEIDOSCOPE_REQUIRED_API_VERSION since it manually checks the version number itself. Otherwise the plugin has no way to indicate it is compatible with version 1 and 2 but not other versions.

@algernon
Copy link
Contributor Author

Would it be possible for the API mismatch error message to also state what the required and observed version numbers are? I think that would add to the helpfulness of the error message.

It would be super helpful, yes, but we can't. When using #error, the string is a literal, we can't have other macros in it, macro concatenation does not work either. So at best, we could hardcode the API version there too, but that feels wrong.

This is probably the desired behavior.

Yes it is. The assumption is that most plugins will support only a single version, and the ones that support more, are an exception. For those, as you write too, the solution is to not use KALEIDOSCOPE_REQUIRED_API_VERSION, but implement the desired check on their own.

@noseglasses
Copy link
Collaborator

What about defining check function macros that operate on KALEIDOSCOPE_API_VERSION under the hood.
Something like

#define KALEIDOSCOPE_REQUIRE_API_VERSION_GREATER(N) \
   static_assert(KALEIDOSCOPE_API_VERSION > N, "...")

static_assert allows for string literal concatenation and nice formatted error messages, see #275.

@algernon
Copy link
Contributor Author

I do not want to introduce any other macro than a strict equality check, but the static_assert idea is neat. Will update the PR after some sleep, thanks!

@noseglasses
Copy link
Collaborator

Yeah. Right. Shouldn't post after so much red wine. The 'greater' is nonsense. The static_assert was the important point, though.

@algernon
Copy link
Contributor Author

The results look neat, thanks @noseglasses!

.../Kaleidoscope/src/Kaleidoscope.h:62:1: error: static assertion failed: Kaleidoscope API version mismatch! We have version 1 available, but version 2 is required.
 static_assert(KALEIDOSCOPE_REQUIRED_API_VERSION == KALEIDOSCOPE_API_VERSION,

Use `static_assert` instead of `#error` to report an API mismatch, resulting in
a much more informative error message.

Thanks to @cdisselkoen for the request, and @noseglasses for the `static_assert`
idea!

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
@obra obra merged commit 086b16f into master Jan 29, 2018
@algernon algernon deleted the f/api-version branch January 29, 2018 14:30
@algernon algernon removed the pending label Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants