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

Implement runtime support for non-exhaustive enums in Java #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Mar 25, 2024

Comment on lines 70 to 71
@Override
public String name() { return null; }
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's beneficial to provide this method in the interface at all if it only makes sense for the Known case. It just seems to encourage null pointer exceptions in user applications.

If the user truly needs a known enum value name, they should first make sure that they really have a known enum value using the instanceof operator. After doing that, they can safely use a type cast to get access to all methods that Java enums have, including name(). I see nothing wrong with this approach.

If they just want to convert the enum value to human readable representation, the Unknown class should actually override Object.toString() so that it shows the integer value it represents (e.g. "42") instead of something like "Unknown@...". Then toString() could be used as a better replacement for name() if the user doesn't want to care if they have a known or unknown enum value.

Speaking of overriding Object methods, Object.toString() is not the only method that should be overridden, as @dgelessus mentioned in kaitai-io/kaitai_struct#778 (comment):

public class UnknownCompressionAlgorithm implements ICompressionAlgorithm {
    ...
    // Insert equals, hashCode, toString, etc. boilerplate here,
    // which need to be implemented manually, unlike in enums.
}

equals and hashCode certainly makes sense - we don't want to use reference equality for Unknown objects. If two different Unknown objects represent the same integer value, then they are obviously equal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

toString is implemented by generator as <kaitai-java-enum-name>(<enum-long-value>).
equals now also implemented by generator (because we do not want that different classes be equal).

hashCode now implemented here as Long.hashCode(this.id).

Comment on lines 29 to 34
* Each generated enum is represented by it's own interface and pair of nested
* classes: enum class and ordinal class:
*
* <code><pre>
* // marker interface
* public interface <kaitai-enum-name> extends IKaitaiEnum {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes sense to include snippets of generated code in runtime library docstrings. I would just remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That code should help coders to understand what structure expect even when they do not see any generated file, but ok. Removed.

Comment on lines 26 to 27
* Interface, implemented by every enum class generated by Kaitai Struct Compiler,
* which provides assess to the underlying integer value.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Interface, implemented by every enum class generated by Kaitai Struct Compiler,
* which provides assess to the underlying integer value.
* Interface implemented by every enum class generated by Kaitai Struct compiler,
* which provides access to the underlying integer value.

I don't really see the point of the "which provides access to the underlying integer value" part, though. It doesn't seem to be a defining characteristic of enums, more like a (obvious) sub-feature. It's probably better to delete it and just leave it at "Interface implemented by every enum class generated by Kaitai Struct compiler."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to have some words to describe not only what it that, but also why it is used. I removed that sentence, but would be glad to hear what can replace it.

@Mingun
Copy link
Contributor Author

Mingun commented Aug 19, 2024

I implemented hashCode(), removed name() and fix documentation. Corresponding equals is generated in each enum by generator.

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.

2 participants