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

Fix the way we trigger an error on Kaleidoscope.use() with V2 API #346

Merged
merged 1 commit into from Jul 25, 2018

Conversation

algernon
Copy link
Contributor

@algernon algernon commented Jul 25, 2018

When using Kaleidoscope.use() and the V1 API is disabled, we want to display an error. The current method of doing that is not reliable, it sometimes works, sometimes will error out even when not using Kaleidoscope.use(). To fix this, make the assert a tiny bit more complicated, so that the compiler won't act too smart.

(Note: I'm not entirely sure why this works, but it appears to do the trick.)

@noseglasses
Copy link
Collaborator

I'm not entirely sure why this works, but it appears to do the trick

Your solution is called deferred instantiation. In your original version the compiler could evaluate the static_assert right away and it would always fire. The modified version requires the Plugin__ template argument to be known, which deferrs the evaluation of the static_assert until the use template function is instanciated.

You could write the whole thing a bit shorter, without always_false as

template <typename Plugin__>
inline void use(Plugin__ first, ...) {
    static_assert(sizeof(Plugin__) < 0, _DEPRECATE(_DEPRECATED_MESSAGE_USE));
}

In this version the sizeof-operator and thus the static_assert can also not be evaluated until Plugin__ becomes known during instanciation of use. Of course the expression sizeof(Plugin__) < 0 is always false.

@algernon
Copy link
Contributor Author

Thanks! I managed to figure out the why, but it's great to know the name, too. And the simplification is <3, will update this PR and MagicCombo too.

Much appreciated!

When using `Kaleidoscope.use()` and the V1 API is disabled, we want to display
an error. The current method of doing that is not reliable, it sometimes works,
sometimes will error out even when not using `Kaleidoscope.use()`. To fix this,
delay the initialisation of `.use()`, so it only evaluates when used, and thus,
only fails with a descriptive error in that case.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>
@obra obra merged commit 38ccd71 into master Jul 25, 2018
@algernon algernon deleted the h/use-error-fix branch July 25, 2018 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants