Remove member name from transparent associated constants #248
Conversation
grovesNL
force-pushed
the
associated-constants
branch
from
5128653 to
fce3e83
Compare
|
The error handling seems a bit wonky (hard to catch the actual non-error code path by the eye), but the logic looks good to me. |
|
It definitely seems workable, but I'm concerned how big that Vec can grow. Not sure we can avoid it since as you say the order isn't guaranteed. |
Agreed, I tried a couple approaches to simplify some of the
I guess an alternative is to simply loop through everything again, checking |
|
@eqrion ping |
src/bindgen/parser.rs
Outdated
| } | ||
| } else { | ||
| error!( | ||
| "Cannot find type for {}::{} (impl blocks require the struct declaration to be known).", |
There was a problem hiding this comment.
Was it possible for users to have associated constants for enums and not just structs? It looks like the code didn't check for this constraint before, but I may be missing it.
If so, then this is kind of a breaking change. Not sure if that matters or not.
There was a problem hiding this comment.
Hmm good point – I guess we should handle associated constants for enums and traits too.
There was a problem hiding this comment.
It would just be good to not have a breaking change here change. As it seems we allowed them on enums and traits, but didn't really expect them.
There was a problem hiding this comment.
This now should handle the case where an associated constant of a transparent struct has been added to the impl of an enum. I assume this is the case we want to preserve and not repr(transparent) on the enum itself?
I also tried adding it to traits but it doesn't seem like there is much in place for handling traits yet, (unless I've missed it). For example, I couldn't see how to know whether a trait has been implemented for a particular type, and without that I can't emit the associated constant for the concrete type.
src/bindgen/parser.rs
Outdated
| ); | ||
| } | ||
| } | ||
| impls.push(item_impl); |
There was a problem hiding this comment.
Can we minimize our memory usage here by filtering out Impls that don't contain associated constants?
I'm not too worried about it though, as IIRC rustc needs to hold the full AST in memory, so if that's too much for us it should be too much for them as well.
There was a problem hiding this comment.
I think this should be handled now.
| @@ -2,4 +2,4 @@ | |||
| #include <stdlib.h> | |||
| #include <stdbool.h> | |||
|
|
|||
| #define Foo_FOO 0 | |||
| #define Foo_FOO 42 | |||
There was a problem hiding this comment.
I'd actually prefer to fail in this case and not try to assume which is better to resolve the conflict, what do you think?
There was a problem hiding this comment.
That sounds reasonable to me. I was just checking to see if the test expectation change was expected.
There was a problem hiding this comment.
I left it for now unless it's preferable to early exit in this case. The current behavior is to emit an error (with error!) and continue.
grovesNL
force-pushed
the
associated-constants
branch
from
fce3e83 to
7c97da1
Compare
|
@eqrion I updated the PR. Please take a look, thanks! |
|
Looks good, thanks! |
This attempts to fix
repr(transparent)for associated constants by unwrapping the first field on structs (see #238 (comment) for more context).I didn't see obvious ways to infer whether the struct the
implblock corresponds with isrepr(transparent), and the order isn't guaranteed (implprior tostruct). So we have to parse the struct declaration first, and reference it later, which is why theimplblocks are loaded after the regular loop now.I'm not sure whether the general approach is correct, so any feedback is greatly appreciated, especially ideas to simplify the branching or remove the
Vecallocation.