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

Key union converted to a proper class #720

Merged
merged 1 commit into from
Nov 21, 2019

Conversation

noseglasses
Copy link
Collaborator

@noseglasses noseglasses commented Nov 12, 2019

Important: This PR introduces a breaking change. All code that accesses the Key::raw member directory will break.

Travis fails because there are two more PRs that need to be merged along with this to
allow the entire firmware to build.

keyboardio/Kaleidoscope-HIDAdaptor-KeyboardioHID#21
keyboardio/Model01-Firmware#92

Unions are a C-reminiscense that are better avoided in modern C++.
They cause specific problems due to their nature of representing
independent types. The way they are used in Kaleidoscope, they
can easily be replaced by a class.

This enables it to properly work with Key objects in constexpr context
where with the old union-based implementation the compiler reported
errors when one Key was constructed based on a key_code/flags pair and
another one through raw-data. In such a case, the compiler assumes that
both Key instances represent something entirely different. This is
because unions were never meant for type conversions and the C++
standard considers their use for that purpose as undefined behavior.

The new class provides accessor methods for raw-data access and for
key_code/flags-data access.

This is a breaking change as it is is not possible to replace direct
member access patterns like

key.raw = 0xFFFF;

based on the raw-accessors.

All direct access via .raw, .keyCode and .flags have been replaced
throughout Kaleidoscope.

Signed-off-by: Florian Fleissner florian.fleissner@inpartik.de

@obra
Copy link
Member

obra commented Nov 12, 2019

This being a breaking change that touches user sketches without any deprecation period, warning, or back compat makes me fairly nervous.

Is it -possible- to turn the previous union into a class and still use the undefined-but-supported-by-gcc behavior inside to expose the old raw, flags, and keyCode properties for some period of time?

@noseglasses
Copy link
Collaborator Author

This being a breaking change that touches user sketches without any deprecation period, warning, or back compat makes me fairly nervous.

I definitely expected that. Very understandable.

Is it -possible- to turn the previous union into a class and still use the undefined-but-supported-by-gcc behavior inside to expose the old raw, flags, and keyCode properties for some period of time?

This is something that gave me headaches for a long time. Still I haven't found a good solution.

This is not about giving the Key data structure accessors or the actual conversion. It is about the compiler throwing errors as soon as two constexpr Key instances are compared, one having been initialized through raw data and the other one through keyCode and flags. For the compiler these are entirely independent data structures. This happens only at compile time and that's why it did not cause too much trouble up to now. Only when I started working on host_keymaps that heavily rely on constexpr, it turned out that it is impossible with the union-based Key. So technically it's not a undefined-but-supported-by-gcc behavior, depending on what you do with Key.

I am agonizing over a solution that gives us both, the backward compatible behavior and still the ability to do fancy compile time things, yet without success.

@noseglasses
Copy link
Collaborator Author

There's definitely no way to replace a union by a class without breaking anything.

But at a closer look on the use cases of class Key, it turns out that there is a much higher probability for the .keyCode and .flags members being used in user code than the .raw member.

This suggests that we could make the effects of the union->class transition for Key less drastically by keeping .keyCode and .flags and only sacrificing .raw.

Code that uses the .raw member directly will still break.

i force-pushed a change to this PR that replaces Keys data members keyCode and flags with proxy objects that allow for emitting deprecation warnings only in those places where .keyCode and .flags are used from outside of the Key class. This solution is less noisy than just deprecating the class data members directly.

As in contrast to my first approach this solution requires storing uint8_t values for key codes and flags separately instead of packing it into one uint16_t, this means that when reading a Key from PROGMEM, both values have to be read separately. In all places where Keys used to loaded from PROGMEM, a convenient member function Key Key::readFromProgmem() wraps PROGMEM access nicely.

@obra, I am sorry that I cannot reduce your level of nervousity to 0%. But maybe the proposed solution is acceptable, nonetheless. The perspective of obtaining the ability to do all sorts of fancy things with Key at compile time (which the compiler disallows with unions) IMHO is definitely worth it.

@noseglasses noseglasses changed the title WIP: Key union converted to a proper class Key union converted to a proper class Nov 13, 2019
@noseglasses
Copy link
Collaborator Author

Factored out an unrelated change and moved it over to #721. Force pushed changes.

@noseglasses noseglasses force-pushed the pr_keycode_redesign branch 2 times, most recently from 0a18051 to c231589 Compare November 19, 2019 08:47
@noseglasses
Copy link
Collaborator Author

Heureka! Last night I had an epiphany.

There is a way to support Key::keyCode, Key::flags and Key::raw event when moving away from a union to a class.

The trick is to make Key::raw a static member and emit a deprecation message that informs the user about all access to Key::raw having to be replaced by Key::getRaw() and Key::setRaw().

To be on the safe side, no instance for Key::raw will be provided and the code will fail to link until all access to Key::raw has been properly replaced.

Access to Key::keyCode and Key::raw will trigger a normal deprecation warning.

Now we have a solution where users are properly warned and we still finally have a class Key.

@obra
Copy link
Member

obra commented Nov 19, 2019 via email

@obra
Copy link
Member

obra commented Nov 19, 2019

(I'm reviewing the code now)

@obra
Copy link
Member

obra commented Nov 19, 2019

I'd love to get @algernon's take on this before merge.

@algernon
Copy link
Contributor

I'll go over this first thing tomorrow. Already started, but won't be able to finish before I fall asleep.

@noseglasses noseglasses mentioned this pull request Nov 20, 2019
@algernon
Copy link
Contributor

Right... I promised a review, but today didn't quite go as originally planned, so all I can offer is a first impression. I definitely like the direction this is taking, but it is a breaking change, even with the flags and keyCode deprecations. Though, at least the breakage is descriptive, but it is still one. Like @obra, I can live with that. I feel like we're nearing a point where we should consider a flag day, but that's beyond this PR.

What I love about this PR, is Key::readFromProgmem(). DataProxy is also very educational to read. It's also very nice to be able to use a constructor for Keys.

All in all, my first impression is to stamp my approval. We'll need to update NEWS.md and UPGRADING.md too, but I can do that step (but if I don't have to, that's even better!).

Copy link
Contributor

@algernon algernon left a comment

Choose a reason for hiding this comment

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

stamp of approval

I did leave two small comments about the text of the deprecation, but those should not hold up merging this PR, we can quickly fix the texts after, and add an example or two to UPGRADING.md.

#define _DEPRECATED_MESSAGE_DIRECT_KEY_MEMBER_ACCESS \
"Direct access to `Key` class' data members is deprecated.\n" \
"Please use `Key::setKeyCode()`/`Key::getKeyCode()` or\n" \
"`Key::setFlags()`/`Key::getFlags()` instead."
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be reworded a bit. It's clear to anyone who knows C++, but for someone who's just copy & pasting code, this might as well be Klingon. I think a better way would be to point them to UPGRADING.md, where we can provide a few examples of how to get from the old style to the new one.

To be honest, just slap For further information and examples on how to do that, please see UPGRADING.md.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Modified deprecations.h, now pointing to UPGRADING.md.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now, as even Duolingo supports Klingon, one can expect more and more Klingon speaking users around 😏

"use `Key::setRaw()`/`Key::getRaw()` to set and get raw data.\n" \
"\n" \
"Important: This is not a deprecation. Your code will compile but fail\n" \
" to link until all access to `Key::raw` has been replaced."
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, slapping the same text at the end of this should do the trick, and point people to the right direction even better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Modified deprecations.h, now pointing to UPGRADING.md.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
joehendrix Joe Hendrix
Unions are a C-reminiscense that are better avoided in modern C++.
They cause specific problems due to their nature of representing
independent types. The way they are used in Kaleidoscope, they
can easily be replaced by a class.

This enables it to properly work with Key objects in constexpr context
where with the old union-based implementation the compiler reported
errors when one Key was constructed based on a key_code/flags pair and
another one through raw-data. In such a case, the compiler assumes that
both Key instances represent something entirely different. This is
because unions were never meant for type conversions and the C++
standard considers their use for that purpose as undefined behavior.

The new class provides accessor methods for raw-data access and for
key_code/flags-data access.

This is a breaking change as it is is not possible to replace direct
member access patterns like

key.raw = 0xFFFF;

based on the raw-accessors.

For the .keyCode and .flags members, proxy objects are used
to enable the generation of suitable deprecations warnings.

All direct access via .raw, .keyCode and .flags have been replaced
throughout Kaleidoscope.

Information on how to upgrade is provided in UPGRADING.md

Signed-off-by: Florian Fleissner <florian.fleissner@inpartik.de>
@noseglasses
Copy link
Collaborator Author

Thanks @algernon for the review and the positive comments. I totally forgot about UPGRADING.md and NEWS.md. Updated both and fixed the deprecation warnings accordingly.

@obra
Copy link
Member

obra commented Nov 21, 2019

I'm +1 to merge once @algernon signs off.

Copy link
Contributor

@algernon algernon left a comment

Choose a reason for hiding this comment

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

Good job on the docs!

@algernon algernon merged commit 425e069 into keyboardio:master Nov 21, 2019
@noseglasses noseglasses deleted the pr_keycode_redesign branch November 22, 2019 11:47
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.

3 participants