Make ty params non-positional #2364
Comments
ghost
assigned 
|
I had thought to do this as part of #2372. I am debating a somewhat different approach, though, where we have two sets of type parameters (say type/method) and still index positionally? The only real reason to do is performance, though, so I think I will try both ways and do some measurements. Using a id-type mapping would be cleaner. |
|
Positional type parameters are nice for performance. Possibly best to fix the concatenation issue directly and not move to a map. |
|
I've kind of stalled on implementing this. I was experimenting with various alternatives but none of them seem to be working out as cleaner overall, so I think I agree with you @pcwalton . |
|
@nikomatsakis Were you able to do a performance comparison for any of the alternatives? |
|
No, and I doubt that it's significant. It's more that the code wasn't turning out cleaner; actually the opposite in some cases. |
|
Well, I just meant that if performance was the same, that would be a case for continuing to think about it. If performance was a lot worse with your changes, then I would be inclined to agree with you and Patrick. But I still think positional ty params are really error-prone... I'd at least want to know what percentage of compile time type substitution takes before going along with "keep it this way for good performance". |
|
I don't know. @pcwalton could be right that perf is bad, I never got far enough that things built. I just felt like I was making a lot of changes but it wasn't that things were getting simpler---and sometimes they felt more complex. Feel free to pursue it on your own if you like, I'm just not interested in making this change anymore. :) |
|
Surely a class' methods can't be parameterized independently of the class itself, can they? That was something we tried to avoid in the design, I thought.. In any case, deferring this to 0.4. Doesn't sound urgent. |
|
@graydon I am not sure what you mean. Currently, both classes and methods can have type parameters. I was initially opposed to this but it turns out to be extremely useful.... (you can't implement something like |
|
I'm feeling pretty apathetic about this. |
…alfJung Error patterns can be regexes fixes rust-lang#2362
… also include ZSTs (rust-lang#2794) This change considers another case for the code generation of scalar data. In particular, when the expected type is an ADT, we previously considered two cases: either there is no field or there is one. But in cases where the ADT includes a ZST, the ADT will contain multiple fields: one associated to the scalar data, and other fields associated to the ZSTs. In those cases, we ended up crashing as in rust-lang#2680: ```rust error: internal compiler error: Kani unexpectedly panicked at panicked at cprover_bindings/src/goto_program/expr.rs:804:9: assertion failed: `(left == right)` left: `2`, right: `1`: Error in struct_expr; mismatch in number of fields and values. StructTag("tag-_161424092592971241517604180463813633314") [Expr { value: IntConstant(0), typ: Unsignedbv { width: 8 }, location: None, size_of_annotation: None }]. thread 'rustc' panicked at cprover_bindings/src/goto_program/expr.rs:804:9: assertion failed: `(left == right)` left: `2`, right: `1`: Error in struct_expr; mismatch in number of fields and values. StructTag("tag-_161424092592971241517604180463813633314") [Expr { value: IntConstant(0), typ: Unsignedbv { width: 8 }, location: None, size_of_annotation: None }] ``` With the changes in this PR, that's no longer the case. Resolves rust-lang#2364 Resolves rust-lang#2680
@nikomatsakis and I agreed that it would be better to change the
ty::subststype to have thetpsfield be a map from node IDs to types, rather than a vector of types as it is now. This allows ty params to be referred to based on their node IDs rather than positionally as they are now. Among other things, this addresses a bug in the code that I'm trying to isolate in which class ty params get concatenated with method ty params and sometimes a param gets duplicated. Niko said he would do this as a byproduct of rewriting the code for self types.The text was updated successfully, but these errors were encountered: