Give vtables and anonymous items more stable generated names (fixes #60) #110
Conversation
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon. |
There was a problem hiding this comment.
The approach looks good to me.
Note that this now makes the ID depend on the order the items were generated, but that'd be also true with the approach I proposed in #60 (and the result from this looks nicer IMO).
Looks like there's an edge case you might have missed, but apart from that it looks good. I need to look into caching the canonical name in a refcell, since it's becoming increasingly complex, but this is not this PR's problem :)
| /// for anonymous items. Lazily initialized in local_id(). | ||
| local_id: Cell<Option<usize>>, | ||
| /// The next local id to use for a child. | ||
| next_child_local_id: AtomicUsize, |
There was a problem hiding this comment.
Can we use a Cell instead of an atomic? Bindgen is single-threaded.
| ItemKind::Module(..) => (&*RE_ENDS_WITH_BINDGEN_MOD, "mod"), | ||
| _ => (&*RE_ENDS_WITH_BINDGEN_TY, "ty"), | ||
| }; | ||
| match (parent_name, base_name) { |
There was a problem hiding this comment.
You need to take into account that parent might be empty, see tests/headers/template_alias*.
|
Oh, and the part of recognising the On the other hand, it might be easier to do as a post-processing pass before codegen. For example, if we see an |
heycam
force-pushed
the
stable-gen-names
branch
from
53c5e2c to
a3ee7ee
Compare
|
I made a small update: now I only use local_id() if we're generating a name for an enum, struct, class or union. That's because some other types, like pointers inside template parameters, were causing canonical_name() to be called, bumping the local id, even though we didn't end up outputting a declaration. |
heycam
force-pushed
the
stable-gen-names
branch
from
a3ee7ee to
55e7d05
Compare
|
@bors-servo: r+ Thanks @heycam! :) |
|
📌 Commit 55e7d05 has been approved by |
|
⚡ Test exempted - status |
Give vtables and anonymous items more stable generated names (fixes #60) r? @emilio This works pretty well. There are two remaining things in stylo's structs files that have identifiers that look like they won't be that stable: the anonymous enum for the NODE_* flags at the top level, and the `typedef union { ... } nsStyleUnion`. There are various anonymous enums and other things at the top level in system headers that cause these identifiers to have generated IDs in them higher than 1 and 2. Probably for anonymous enums we could just avoid generating a rust enum altogether, since having the static consts should be sufficient. I tried to mess with the codegen to automatically treat `typedef union { ... } nsStyleUnion` like `union nsStyleUnion { ... }` but it seems the way clang exposes the typedef and union are as two adjacent cursors rather than a parent-child relationship, so it's not so easy.
r? @emilio
This works pretty well. There are two remaining things in stylo's structs files that have identifiers that look like they won't be that stable: the anonymous enum for the NODE_* flags at the top level, and the
typedef union { ... } nsStyleUnion. There are various anonymous enums and other things at the top level in system headers that cause these identifiers to have generated IDs in them higher than 1 and 2.Probably for anonymous enums we could just avoid generating a rust enum altogether, since having the static consts should be sufficient. I tried to mess with the codegen to automatically treat
typedef union { ... } nsStyleUnionlikeunion nsStyleUnion { ... }but it seems the way clang exposes the typedef and union are as two adjacent cursors rather than a parent-child relationship, so it's not so easy.