[L] Change in dart/sdk[main]: [DAS] Fixes Create setters, getters and fields for using type parameters
Felipe Morschel (Gerrit)
Felipe Morschel has uploaded the change for review![]( "Open in Gerrit")
Commit message
[DAS] Fixes Create setters, getters and fields for using type parameters
Change information
- M pkg/analysis_server/lib/src/services/correction/dart/create_field.dart
- M pkg/analysis_server/lib/src/services/correction/dart/create_getter.dart
- M pkg/analysis_server/lib/src/services/correction/dart/create_setter.dart
- M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
- M pkg/analysis_server/test/src/services/correction/fix/create_field_test.dart
- M pkg/analysis_server/test/src/services/correction/fix/create_getter_test.dart
- M pkg/analysis_server/test/src/services/correction/fix/create_setter_test.dart
- M pkg/analysis_server_plugin/lib/edit/dart/correction_producer.dart
- A pkg/analyzer/lib/src/utilities/extensions/dart_type.dart
- M pkg/analyzer_plugin/lib/src/utilities/change_builder/change_builder_dart.dart
- M pkg/analyzer_plugin/lib/utilities/change_builder/change_builder_dart.dart
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
This is only part of https://dart-review.googlesource.com/c/sdk/+/442240. The change was becomming way too big for a single CL so this is the start it. Once this lands I have some other stashed changes to rebase and upload on new CLs.
Once I get your approval I'll add someone from the analyzer team to review the newly added file with the extension methods. I'm still not convinced that the naming there is the best I could get but the tests are now passing.
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 8 comments![]( "Open in Gerrit")
I haven't checked everything yet, but I thought there were enough comments that I should let you start working on them.
dynamic field;I believe that the implicit bound of a type parameter is `Object?`, so we should consider using that here rather than `dynamic`.
if (expression.enclosingExecutableElementExecutable elements are not closures. We might need to change the name of this method to reflect the new semantics.
TypeParameterType? isBoundTo(DartType? other) {I'm not sure what this method is trying to do. Could you provide documentation, and maybe a name that doesn't start with 'is' (because that makes me think it should return a `bool`.
DartType? thisOrBound(bool useThis) {I'm not sure I understand the purpose of this parameter. It looks to me like the method is returning the receiver unless the receiver is a type parameter that isn't in scope. But if [useThis] is `true`, then this method should not have been invoked because the receiver is already known to be a type parameter that is in scope.
And if we already know before we invoke this method whether the receiver is a type parameter, this method just duplicates a check we've already made.
(It might have made more sense if there'd been a doc comment.)
return bound;This isn't valid. The bound of a type parameter can be another type parameter. We really need to iterate until we get to a bound that isn't a type parameter. (Or until we get to a type parameter that we've already seen, because in invalid code we could have an infinite loop if we don't check for that case.
bool alwaysWriteType = false,In all of the call sites I can find, this value is always computed the same way (by checking for a lint enablement). I'd prefer to remove this parameter and to have the change builder automatically do the right thing by checking the lint's enablement.
ExecutableElement? methodBeingCopied,It took me a while to figure out what's going on here. In part, at least, because I was thrown by the name.
What the method `writeType` really needs is a way to determine, given a `type` that is either a type parameter or is a parameterized type with a type parameter somewhere inside the type arguments, whether that type parameter is in scope where the type is to be written.
The original use case was copying a method, and by passing in the method being copied `writeType` could determine whether a type parameter that was used in the `type` was going to be in scope because it would be defined by the newly created method. That's the only case it handled, and that's not the case you need here.
What we need is to re-write `writeType` to have a more general way of answering the question of whether a type parameter is in scope. For the old use case we can just pass in the type parameters defined by the original method (instead of passing in the whole method). In the case of creating a new member declaration we need to pass in the type parameters of the class (or class-like declaration) to which the declaration is being added.
Unfortunately, `writeType` is a public API, so we'll need to deprecate the old `methodBeingCopied` parameter and add a new parameter (`typeParametersInScope`?) and support both for some hopefully short period of time.
Maybe we could make that be a separate CL (I expect that there are lots of uses of `writeType` that will need to be cleaned up.)
Related details
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
It took me a while to figure out what's going on here. In part, at least, because I was thrown by the name.
What the method `writeType` really needs is a way to determine, given a `type` that is either a type parameter or is a parameterized type with a type parameter somewhere inside the type arguments, whether that type parameter is in scope where the type is to be written.
The original use case was copying a method, and by passing in the method being copied `writeType` could determine whether a type parameter that was used in the `type` was going to be in scope because it would be defined by the newly created method. That's the only case it handled, and that's not the case you need here.
What we need is to re-write `writeType` to have a more general way of answering the question of whether a type parameter is in scope. For the old use case we can just pass in the type parameters defined by the original method (instead of passing in the whole method). In the case of creating a new member declaration we need to pass in the type parameters of the class (or class-like declaration) to which the declaration is being added.
Unfortunately, `writeType` is a public API, so we'll need to deprecate the old `methodBeingCopied` parameter and add a new parameter (`typeParametersInScope`?) and support both for some hopefully short period of time.
Maybe we could make that be a separate CL (I expect that there are lots of uses of `writeType` that will need to be cleaned up.)
Completed at https://dart-review.googlesource.com/c/sdk/+/442282
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 7 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
7 comments
bound.ifTypeOrNull<TypeParameterType>()?.element.wrapInList(),I dislike this. I want to replace this expression with a null-aware-element list. I'll rebase this and refactor (here and for field) once the version bump happens.
Do you plan on making the version-bump change @sraw...@google.com?
I believe that the implicit bound of a type parameter is `Object?`, so we should consider using that here rather than `dynamic`.
Handled internally by `DartEditBuilderImpl` now. Thanks!
Executable elements are not closures. We might need to change the name of this method to reflect the new semantics.
Thanks for noting!
I'm not sure what this method is trying to do. Could you provide documentation, and maybe a name that doesn't start with 'is' (because that makes me think it should return a `bool`.
I think this is better now, please let me know if you still think it could be better. Thanks!
I'm not sure I understand the purpose of this parameter. It looks to me like the method is returning the receiver unless the receiver is a type parameter that isn't in scope. But if [useThis] is `true`, then this method should not have been invoked because the receiver is already known to be a type parameter that is in scope.
And if we already know before we invoke this method whether the receiver is a type parameter, this method just duplicates a check we've already made.
(It might have made more sense if there'd been a doc comment.)
I've done some refactoring after that other CL and this method is not needed anymore so I'll skip these comments. If you still find inconsistencies, please let me know!
This isn't valid. The bound of a type parameter can be another type parameter. We really need to iterate until we get to a bound that isn't a type parameter. (Or until we get to a type parameter that we've already seen, because in invalid code we could have an infinite loop if we don't check for that case.
See my answer to https://dart-review.googlesource.com/c/sdk/+/442240/comment/32668a79_d1128de3/. Thanks!
In all of the call sites I can find, this value is always computed the same way (by checking for a lint enablement). I'd prefer to remove this parameter and to have the change builder automatically do the right thing by checking the lint's enablement.
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
bound.ifTypeOrNull<TypeParameterType>()?.element.wrapInList(),I dislike this. I want to replace this expression with a null-aware-element list. I'll rebase this and refactor (here and for field) once the version bump happens.
Do you plan on making the version-bump change @sraw...@google.com?
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
Can I get new reviews for this please? Thanks a lot!
bound.ifTypeOrNull<TypeParameterType>()?.element.wrapInList(),Samuel RawlinsI dislike this. I want to replace this expression with a null-aware-element list. I'll rebase this and refactor (here and for field) once the version bump happens.
Do you plan on making the version-bump change @sraw...@google.com?
I do plan on making this soon. Maybe today.
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
bound.ifTypeOrNull<TypeParameterType>()?.element.wrapInList(),Samuel RawlinsI dislike this. I want to replace this expression with a null-aware-element list. I'll rebase this and refactor (here and for field) once the version bump happens.
Do you plan on making the version-bump change @sraw...@google.com?
Felipe MorschelI do plan on making this soon. Maybe today.
Thanks!
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
bound.ifTypeOrNull<TypeParameterType>()?.element.wrapInList(),Samuel RawlinsI dislike this. I want to replace this expression with a null-aware-element list. I'll rebase this and refactor (here and for field) once the version bump happens.
Do you plan on making the version-bump change @sraw...@google.com?
Felipe MorschelI do plan on making this soon. Maybe today.
Samuel RawlinsThanks!
This is done.
Yes and I've pushed a new patch with the refactoring already. Thanks!
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
I've rebased this one. Can I get some reviews here please? Thanks!
Friendly ping @sraw...@google.com and @brianwi...@google.com
TypeParameterType? boundFor(DartType? other) {Can I get any thoughts on this method and its doc, please? Thanks!
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted Auto-Submit+1![]( "Open in Gerrit")
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 8 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
8 comments
Code looks good to me. +1 so I can run bots.
// else if (target case Identifier(:InstanceElement element)) {To delete?
var index =I think this statement is a long way to do `List.indexOf` and `List.indexWhere`, yes? You don't get the nice `??` ability with `-1` if an item is not found, but I think the improved readability is worth that shorthand.
} else if (self case FunctionType(Since we get type promotion on `self`, I think it's easier to just use `else if (self is FunctionType)` and `self.returnType`, `self.typeParameters`, and `self.formalParameters`.
.where((type) => type == element)This looks suspect. This looks like we can just use `element` directly, if `typeParameters.contains(element)`.
And `whereType<TypeParameterType>()` is then not needed. I get the brevity of returning a single (8-line) statement. But I think breaking this up into a few statements using `contains` would make the code more readable.
return fields.map((field) => field.type.boundFor(other)).firstOrNull;`.map.firstOrNull` doesn't look performant. What about
`fields.firstOrNull?.map((f) => f.type.boundFor(other))`
/// Returns whether the type was written.I think import to emphasize it does the writing, maybe "Writes [type] if it should be, and returns whether it was written."
if (_canWriteType(Just for readability, can we break this up into some early returns? Something like:
```dart
// this is the cheaper check
if (!shouldWriteDynamic && visibleType is DynamicType) return false;
if (!_canWriteType(...)) return false;
//...
```
Related details
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 7 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
7 comments
// else if (target case Identifier(:InstanceElement element)) {Felipe MorschelTo delete?
Yes, thanks! Forgot about it.
I think this statement is a long way to do `List.indexOf` and `List.indexWhere`, yes? You don't get the nice `??` ability with `-1` if an item is not found, but I think the improved readability is worth that shorthand.
I think I made it better by just iterating on it once and handling it all inside a `for`. If you prefer I use the methods above instead still, please say so. Thanks!
Since we get type promotion on `self`, I think it's easier to just use `else if (self is FunctionType)` and `self.returnType`, `self.typeParameters`, and `self.formalParameters`.
Done
This looks suspect. This looks like we can just use `element` directly, if `typeParameters.contains(element)`.
And `whereType<TypeParameterType>()` is then not needed. I get the brevity of returning a single (8-line) statement. But I think breaking this up into a few statements using `contains` would make the code more readable.
Thanks!
Although I was thinking a bit more about this method and I could not think of a way for a type parameter to be infered by some function or record like this. It would always be resolved normally.
The only case I could think of was classes. I've ended up removing this part and the one for records entirely and all tests pass. If we ever think of a new way this method can be improved we can do so then.
return fields.map((field) => field.type.boundFor(other)).firstOrNull;`.map.firstOrNull` doesn't look performant. What about
`fields.firstOrNull?.map((f) => f.type.boundFor(other))`
Done
I think import to emphasize it does the writing, maybe "Writes [type] if it should be, and returns whether it was written."
Great! Thanks!
Just for readability, can we break this up into some early returns? Something like:
```dart
// this is the cheaper check
if (!shouldWriteDynamic && visibleType is DynamicType) return false;
if (!_canWriteType(...)) return false;
//...
```
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 2 comments![]( "Open in Gerrit")
TypeParameterType? boundFor(DartType? other) {Can I get any thoughts on this method and its doc, please? Thanks!
It took me a while to figure out what this is doing (or at least to think that I understand).
The name of the method is confusing because the method has nothing to do with bounds. And the comment seems wrong to me.
What it's really doing, I think, is returning the type parameter that corresponds to the type argument `other` in `this` type (but only when `other` is a type parameter). Maybe a better name would be `typeParameterCorrespondingTo`.
But if I'm right, then there's a problem. If I have a type `A<B, B>` (for some type parameter `B`), where `A` is declared as `class A<T, U>`, then we can't know whether to return `T` or `U`. I'm guessing that the right semantics is to return `null` when there isn't a unique match, but I haven't verified that.
if (self is InterfaceType) {I think we could remove the need for this check by replacing the `self == null` on line 26 with `self is! InterfaceType`.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
TypeParameterType? boundFor(DartType? other) {Brian WilkersonCan I get any thoughts on this method and its doc, please? Thanks!
It took me a while to figure out what this is doing (or at least to think that I understand).
The name of the method is confusing because the method has nothing to do with bounds. And the comment seems wrong to me.
What it's really doing, I think, is returning the type parameter that corresponds to the type argument `other` in `this` type (but only when `other` is a type parameter). Maybe a better name would be `typeParameterCorrespondingTo`.
But if I'm right, then there's a problem. If I have a type `A<B, B>` (for some type parameter `B`), where `A` is declared as `class A<T, U>`, then we can't know whether to return `T` or `U`. I'm guessing that the right semantics is to return `null` when there isn't a unique match, but I haven't verified that.
Maybe a better name would be `typeParameterCorrespondingTo`.
Thanks! I was struggling a bit to come up with this when working on this originally. I'll be sure to rename and review the comments.
> But if I'm right, then there's a problem. If I have a type `A<B, B>` (for some type parameter `B`), where `A` is declared as class `A<T, U>`, then we can't know whether to return `T` or `U`. I'm guessing that the right semantics is to return `null` when there isn't a unique match, but I haven't verified that.
I guess so too, I'd not thought of this case. I'll be sure to add a test case and verify we are doing the correct thing here. This would fall-back to the bound on `B` so we should be safe.
I think we could remove the need for this check by replacing the `self == null` on line 26 with `self is! InterfaceType`.
Great idea! Originally I was having some trouble with the semantics here and I did some work for `RecordType` and `FunctionType` but they can't do the same here.
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Commit-Queue | +1 |
1 comment
T? g<T>(A a) => a.field;The resulting code generates a diagnostic:
A value of type 'Object?' can't be returned from the function 'g' because it has a return type of 'T?'. (return_of_invalid_type)
The only solution I can think of is to add a cast, which I don't think we want to do as part of generating the field.
I suppose the right way to think of this is that it's better to offer help getting closer to an eventual solution even if it's just trading one diagnostic for another than it is to offer no help at all?
Note that the next test has the same problem.
Related details
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
T? g<T>(A a) => a.field;The resulting code generates a diagnostic:
A value of type 'Object?' can't be returned from the function 'g' because it has a return type of 'T?'. (return_of_invalid_type)
The only solution I can think of is to add a cast, which I don't think we want to do as part of generating the field.
I suppose the right way to think of this is that it's better to offer help getting closer to an eventual solution even if it's just trading one diagnostic for another than it is to offer no help at all?
Note that the next test has the same problem.
I'd prefer having the assist rather than having none because this way, I can continue my train of thought. It just so happens that there is no good way for the tooling to ever suggest a good fix here that is not `dynamic` and this is not something that would work in strict mode either so I'd say to leave this as is.
We even have two different fixes for this new diagnostic:
- Change the return type
- Add cast
So the user will be well served of tolling fixes here for this whole process.
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
I've rebased this one. Can I get some reviews here please? Thanks!
Friendly ping @sraw...@google.com and @brianwi...@google.com
Acknowledged
T? g<T>(A a) => a.field;Felipe MorschelThe resulting code generates a diagnostic:
A value of type 'Object?' can't be returned from the function 'g' because it has a return type of 'T?'. (return_of_invalid_type)
The only solution I can think of is to add a cast, which I don't think we want to do as part of generating the field.
I suppose the right way to think of this is that it's better to offer help getting closer to an eventual solution even if it's just trading one diagnostic for another than it is to offer no help at all?
Note that the next test has the same problem.
I'd prefer having the assist rather than having none because this way, I can continue my train of thought. It just so happens that there is no good way for the tooling to ever suggest a good fix here that is not `dynamic` and this is not something that would work in strict mode either so I'd say to leave this as is.
We even have two different fixes for this new diagnostic:
- Change the return type
- Add cast
So the user will be well served of tolling fixes here for this whole process.
- Felipe Morschel
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
1 comment
TypeParameterType? typeParameterCorrespondingTo(DartType? other) {Yes! Better name and better docs. 👍
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Commit-Queue+1![]( "Open in Gerrit")
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 2 comments![]( "Open in Gerrit")
TypeParameterType? typeParameterCorrespondingTo(DartType? other) {This is a quite wide extension doing something specific for quick fixes.
And because it is wide, it will fire on every type in the analyzer, without being useful.
It probably does not belong to the analyzer package..
} else if (current.typeParameterCorrespondingTo(other) != null) {I don't understand this branch, it looks that it is not documented above.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
TypeParameterType? typeParameterCorrespondingTo(DartType? other) {This is a quite wide extension doing something specific for quick fixes.
And because it is wide, it will fire on every type in the analyzer, without being useful.It probably does not belong to the analyzer package..
Alright, then. I'll move into the `analysis_server` and defer to Sam and Brian if they agree it should be there. Thanks!
} else if (current.typeParameterCorrespondingTo(other) != null) {I don't understand this branch, it looks that it is not documented above.
Oh, I don't think it is necessary. I've removed it and no test failed. I'll remove this. Thanks!
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |