Skip to content

Conversation

@basil-cow
Copy link
Contributor

@basil-cow basil-cow commented Apr 6, 2020

EDIT: doesnt actually close \#261, we need #368 to land first and address it

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Seems good! I left some nits.

}
}

fn add_sized_program_clauses<I: Interner>(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should make builtin_traits/sized.rs and put this function in there


let struct_datum = db.struct_datum(struct_id);

if struct_datum.binders.map_ref(|b| b.fields.is_empty()).value {
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment:


Structs with no fields are always Sized

return;
}

let last_field_ty = struct_datum
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a comment here -- something like this:


To check if a struct type S<..> is Sized, we only have to look at its last field. This is because the WF checks for structs require that all the other fields must be Sized.

@basil-cow basil-cow marked this pull request as ready for review April 7, 2020 04:34
@basil-cow
Copy link
Contributor Author

Addressed the nits and rebased

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Thanks! One minor suggestion but otherwise I think looks good to go

@basil-cow
Copy link
Contributor Author

All done and rebased

@nikomatsakis nikomatsakis merged commit 73a74be into rust-lang:master Apr 7, 2020
@basil-cow
Copy link
Contributor Author

@nikomatsakis wait, why did I put the constraint check into the WfSolver? It should probably be in ToProgramClauses for StructDatum, right?

@basil-cow
Copy link
Contributor Author

made a zulip thread because I have many questions about this

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.

2 participants