Skip to content

Conversation

jcollins-g
Copy link
Contributor

Fixes #1445. Adding Paul to the reviewers as a comment in ConstFieldElementImpl suggested he might have context as to whether the change to how we compute enum indexes is actually an improvement over some variant of the original hack.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Jul 10, 2017
@jcollins-g
Copy link
Contributor Author

AppVeyor builds are pub errors and I don't have access to retry them. @devoncarew?

newModelElement = new EnumField(e, library, getter, setter);
if (e.enclosingElement.isEnum) {
int index =
e.computeConstantValue()?.getField('index')?.toIntValue();
Copy link
Member

Choose a reason for hiding this comment

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

This definitely works but it took a bit of thinking before I understood why. Essentially what's happening here is that the fields in an enum class E are all static fields having type E, except for the two "special" fields index (a non-static field having type int) and values (a static field having type List<E>). You only want your new behavior for the non-special fields, and it works because only the type E has a non-static member index whose value is an int; the types int and List<E> don't.

If I'm understanding the code correctly there's a getter FieldElement.isEnumConstant that does what you want. So I think you could replace lines 2140-2161 with something like:

if (e.isEnumConstant) {
  int index = e.computeConstantValue().getField('index').toIntValue();
  newModelElement = new EnumField.forConstant(index, e, library, getter);
} else if (e.enclosingElement.isEnum) {
  newModelElement = new EnumField(e, library, getter, setter);
} else {
  newModelElement = new Field(e, library, getter, setter);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's so much better. This logic is what I intended: if I had seen/thought to look for e.isEnumConstant I would have written it exactly like this. Thanks!

Copy link
Member

@stereotype441 stereotype441 left a comment

Choose a reason for hiding this comment

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

I made a comment about how enum values are computed. I am not too familiar with DartDoc so I'll leave the full review to Keerti.

..addAll(constructors)
..addAll(staticMethods)
..addAll(staticProperties)
..addAll(allInstanceMethods)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was a duplicate, it was already addAll on line 454.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that line is not shown in the review :)

@jcollins-g jcollins-g merged commit fb2c2d0 into master Sep 11, 2017
@jcollins-g jcollins-g deleted the null-enums branch September 12, 2017 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants