Whitelist for slice args that do not need to be mutable #1199
Conversation
|
@alexcrichton this PR is now ready for review! I whitelisted all of the WebGlRenderingContext methods and left instructions in comments about how future people (myself.. others..) can add more as we run into them. |
crates/webidl/src/util.rs
Outdated
| /// Here we implement a whitelist for those cases. This whitelist is currently | ||
| /// maintained by hand. | ||
| fn maybe_adjust<'a>(mut idl_type: IdlType<'a>, id: &'a OperationId) -> IdlType<'a> { | ||
| let immutable_f32_fns: Vec<&'static str> = vec![ |
There was a problem hiding this comment.
This plus the HashSet collection below I think can be somewhat costly to do once-per-function, so could this work be executed once and stored into an outer context which is then passed in here?
There was a problem hiding this comment.
I ended up introducing lazy_static so that I didn't have to pass the hashset through.
The downside being an extra dep.
Please let me know if that downside is too great here.
crates/webidl/src/util.rs
Outdated
| } | ||
|
|
||
| // example `op`... -> "vertexAttrib1fv" | ||
| if let OperationId::Operation(Some(op)) = id { |
There was a problem hiding this comment.
This can reduce some indentation as:
let id = match id {
OperationId::Operation(Some(op)) => op,
_ => return idl_type,
};
if !immutable_whitelist.contains(id) {
return idl_type;
}
// ...There was a problem hiding this comment.
First block makes sense!
For the second block I'm thinking that we don't want to return early because in the future we might test for other whitelists besides our F32 whitelist.
Unless we just had one whitelist HashMap that also specified the types to whitelist the function for...
But anyways.. for now I'll adapt this advice!
crates/webidl/src/util.rs
Outdated
| // slice argument immutable. | ||
| if immutable_f32_slice_whitelist.get(op).is_some() { | ||
| if let IdlType::Union(ref mut union) = idl_type { | ||
| if let IdlType::Float32Array { ref mut immutable } = union[0] { |
There was a problem hiding this comment.
Could this instead be something like:
fn flag_slices_immutable(ty: &mut IdlType) {
match ty {
IdlType::Float32Array { immutable } => *immutable = true,
IdlType::Union(list) => {
for item in list { flag_slices_immutable(item); }
}
// ... other recursive cases like Nullable handled here
// catch-all for everything else like Object
_ => {}
}
}
|
@alexcrichton thanks a lot for all of the tips/advice/guidance here - I've applied your feedback and things are now ready for review. |
crates/webidl/src/util.rs
Outdated
| for item in list { flag_slices_immutable(item); } | ||
| } | ||
|
|
||
| // TODO: ... other recursive cases like Nullable handled here |
There was a problem hiding this comment.
I think there's only a few of these other than Union, mind going ahead and expanding them in this PR?
There was a problem hiding this comment.
Ohhh gotcha - will do next time I hop in this branch!
crates/webidl/src/util.rs
Outdated
|
|
||
| lazy_static! { | ||
| /// These functions will have their f32 slice arguments be immutable. | ||
| static ref IMMUTABLE_F32_WHITELIST: HashSet<&'static str> = { |
There was a problem hiding this comment.
Instead of lazy_static!, could this be constructed and perhaps cached in the FirstPassRecord and then passed into maybe_adjust?
There was a problem hiding this comment.
Thanks for the pointer, will do next time I hop in this branch!
|
@alexcrichton all feedback addressed! |
|
Thanks! To avoid the |
|
I noticed that we were using But for all I know the chances of that are next to zero (don't fully know the flow yet). So cool I'll follow suit on how we set the |
|
@alexcrichton done - ready for review |
|
Ah yeah we shouldn't be creating too many instances of this struct but if it comes up I'm sure we can deduplicate as necessary! Thanks! |
Closes #1005