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

Provide an API to constify enum variants. #393

Merged
merged 6 commits into from
Jan 11, 2017

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Jan 10, 2017

@upsuper
Copy link
Contributor

upsuper commented Jan 10, 2017

A question: may I do

/// <div rustbindgen constant></div>
eCSSProperty_COUNT,

rather than adding it after? I'm concerned about exceeding 80 chars limitation in Gecko code...

Also, is it possible to apply <div rustbindgen hide></div> to enum item? I think we don't really need the dummy ones at all.

@emilio
Copy link
Contributor Author

emilio commented Jan 10, 2017

I don't think you can do that because doxygen thinks that kind of comment is a comment for the whole enum and applies them to every following item.

I've added a programmatic API that you could also use from the build script though (see the type_chooser API).

@emilio
Copy link
Contributor Author

emilio commented Jan 10, 2017

Regarding hide, yeah, we should be able to handle that quite easily, let me move some code around :)

@upsuper
Copy link
Contributor

upsuper commented Jan 10, 2017

I guess you can change constify_enum_variant to a more general method and let it return the action for a given enum item?

@emilio
Copy link
Contributor Author

emilio commented Jan 10, 2017

Heh, was writing that right now :)

What do you think about the last commit?

@upsuper
Copy link
Contributor

upsuper commented Jan 10, 2017

Looking at the (previously) last commit, I wonder do we really need to try that hard to add enum variant? I don't see much usefulness from doing so.

I think we can simply not add constified items to enum variants, which should simplify the code logic.

@upsuper
Copy link
Contributor

upsuper commented Jan 10, 2017

Oh, ignore my last comment. I suddently realize that you have to add enum variant anyway, because otherwise that constant wouldn't be referring to an enum variant.

@emilio
Copy link
Contributor Author

emilio commented Jan 10, 2017

@upsuper consider the following:

enum Foo {
    A = 1,
    B = 10, /**< <div rustbindgen constant></div> */
};

If I do that (avoid adding constified items), two things happen.

First, they wouldn't have same type (we'd need to make the const an integer or whatever). Second, rust's optimizations may kick in with disastrous consequences (a la #225).

We could enforce constant to be just used when other enum values are around, but seemed easy enough to support the general case.

@emilio
Copy link
Contributor Author

emilio commented Jan 10, 2017

Oh, didn't read your other comment (had my browser window open).

Copy link
Contributor

@upsuper upsuper left a comment

Choose a reason for hiding this comment

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

r=me with the nits addressed.


while iter.peek().is_some() || !constified_variants.is_empty() {
let variant = iter.next()
.unwrap_or_else(|| constified_variants.remove(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably use VecDeque instead of Vec for constified_variants, and change the three lines above to something like

while let Some(variant) = iter.next().or_else(|| constified_variants.pop_front()) {
    // ...
}

/// You can see the kind of comments that are accepted in the Doxygen
/// documentation:
///
/// http://www.stack.nl/~dimitri/doxygen/manual/docblocks.html
Copy link
Contributor

Choose a reason for hiding this comment

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

The last paragraph as well as the link doesn't seem to be something specific to this field. Probably this is comment which should be added to the Annotation struct.

Also, hide now supports enum variants in addition to types, which should be mentioned in the document of that field.

@emilio
Copy link
Contributor Author

emilio commented Jan 11, 2017

Nice catches :)

@bors-servo r=upsuper

@bors-servo
Copy link

📌 Commit fee7e96 has been approved by upsuper

@bors-servo
Copy link

⌛ Testing commit fee7e96 with merge 6bfeae1...

bors-servo pushed a commit that referenced this pull request Jan 11, 2017
Provide an API to constify enum variants.

r? @upsuper
@bors-servo
Copy link

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit fee7e96 into rust-lang:master Jan 11, 2017
@emilio emilio deleted the enum-const-api branch January 11, 2017 11:26
@emilio emilio mentioned this pull request Jan 24, 2017
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.

4 participants