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.

Sign up for GitHub

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

Jump to bottom

Strongly type ids #1050

Merged
merged 25 commits into from
Oct 2, 2017
Merged

Strongly type ids #1050

merged 25 commits into from
Oct 2, 2017

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Sep 30, 2017 •
edited
Loading

  • TypeId
  • ModuleId
  • VarId
  • FunctionId

@bors-servo
Copy link

☔ The latest upstream changes (presumably #1049) made this pull request unmergeable. Please resolve the merge conflicts.

@fitzgen
Introduce the TypeId newtype over ItemId
2a93916 
This commit also makes `BindgenContext::resolve_type` take a `TypeId`, and adds
unchecked conversions everywhere that we call it. Next, I'm going to go through
the code base, replacing these unchecked conversions with checked ones, and
tightening up types as I go.
@fitzgen
Remove unused parameter to codegen::utils::type_from_named
f5b06ed
@fitzgen
Turn build_templated_path into build_path
dd68ec8 
None of the callers were passing template parameters.
@fitzgen
Resolve an Item with any id type that converts into an ItemId
bd232fb
@fitzgen
Make TypeKind::{Alias,TemplateAlias,Array,Pointer,Reference} use `T…
bd32423 
…ypeId`

This commit makes certain `TypeKind`s that can only reference other types use
`TypeId` instead of `ItemId`.
@fitzgen
Make TemplateInstantiation's definition into a TypeId
25ce0f6 
The definition of a template is always a type, so it should be a TypeId rather
than an ItemId. Template arguments, on the other hand are not guaranteed to be
types. They can be constant values, for example.
@fitzgen
Make functions which take an ItemId generic to take any kind of id
2a0cee7
@fitzgen
Make comp fields contain TypeIds
95d9741
@fitzgen
Make a bunch more methods take generic ids
3e869f1
@fitzgen
Make base members use TypeId rather than ItemId
c2e94be
@fitzgen
instantiate_template should take a TypeId for the template defini…
d83e732 
…tion
@fitzgen
Make Enum::repr into a TypeId
5e75397
@fitzgen
A bunch of parsing things should return TypeId
9ad1e6a
@fitzgen
Turn CompInfo::inner_types into TypeId
c780d94
@fitzgen
Replacement should use TypeId rather than ItemId
9220ffe
@fitzgen
Replace as_type_id_unchecked calls with checked calls where possible
2549d5e
@fitzgen
Introduce ModuleId to strongly type IDs pointing at Modules
c912f55
@fitzgen
Put newtype-of-ItemId boilerplate behind a macro
9b294d3
@fitzgen
Introduce VarId for ids pointing to Var
edee094
@fitzgen
Make CompInfo::inner_vars use VarId instead of ItemId
45db21b
@fitzgen
Introduce the FunctionId newtype for ids pointing to functions
3ef31e8
@fitzgen
Make methods/constructors/destructors use FunctionId
3495d03 
And also allow ID comparison across ID types, as this makes implementing the
above much easier.
@fitzgen
Copy link
Member Author

fitzgen commented Oct 2, 2017

r? @pepyakin

@fitzgen fitzgen changed the title [WIP] Strongly type ids Strongly type ids Oct 2, 2017
pepyakin
pepyakin reviewed Oct 2, 2017
View reviewed changes
src/ir/context.rs Outdated
@@ -1066,15 +1253,15 @@ impl BindgenContext {
}

/// Look up whether the item with `id` has vtable or not.
pub fn lookup_item_id_has_vtable(&self, id: &ItemId) -> bool {
pub fn lookup_item_id_has_vtable<Id: Into<ItemId>>(&self, id: Id) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose has_vtable is defined only for types and calling it on a not type is bit smelly. Would it be hard to change this fn to only accept TypeId?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call -- I'll give it a try.

@fitzgen
Copy link
Member Author

fitzgen commented Oct 2, 2017

@pepyakin OK, I made is_unsized, has_vtable, and has_destructor methods all take TypeId.

@pepyakin
Copy link
Contributor

pepyakin commented Oct 2, 2017

Looks great!

@bors-servo r+

@bors-servo
Copy link

@pepyakin: 🔑 Insufficient privileges: Not in reviewers

@fitzgen
Copy link
Member Author

fitzgen commented Oct 2, 2017

@bors-servo r=pepyakin

Hmm doesn't seem to be deployed yet...

@bors-servo
Copy link

📌 Commit 5f3bf1f has been approved by pepyakin

@bors-servo
Copy link

⌛ Testing commit 5f3bf1f with merge c66598d...

bors-servo pushed a commit that referenced this pull request Oct 2, 2017
Auto merge of #1050 - fitzgen:strongly-type-ids, r=pepyakin
c66598d 
Strongly type ids

* [X] `TypeId`
* [X] `ModuleId`
* [x] `VarId`
* [x] `FunctionId`
@bors-servo
Copy link

💔 Test failed - status-travis

@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: pepyakin
Pushing c66598d to master...

@bors-servo bors-servo merged commit 5f3bf1f into rust-lang:master Oct 2, 2017
@bors-servo bors-servo mentioned this pull request Oct 2, 2017
Merged
@fitzgen fitzgen deleted the strongly-type-ids branch October 2, 2017 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepyakin pepyakin pepyakin left review comments

Assignees

@pepyakin pepyakin

Labels
S-tests-failed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fitzgen @bors-servo @pepyakin @highfive