Skip to content

Conversation

@jarrodcolburn
Copy link

Remove unneeded null check after promotion

Copy link
Author

@jarrodcolburn jarrodcolburn left a comment

Choose a reason for hiding this comment

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

^

@kevmoo
Copy link
Member

kevmoo commented Jan 31, 2023

@lrhn ?

@jarrodcolburn
Copy link
Author

jarrodcolburn commented Jan 31, 2023

Question about list Mixin...
The ListMxin documentation states that all Mixin methods really only on length,operator[] ,add, length= and operator[]=.

/// `ListMixin` can be used as a mixin to make a class implement
/// the `List` interface.
///
/// This mixin implements all read operations using only the `length` and
/// `operator[]` and members. It implements write operations using those and
/// `add`, `length=` and `operator[]=`.
/// Classes using this mixin should implement those five operations.

My ((in)correct?) understand of this is... no public method defined on the mixin calls another (public method). This ensures later classes that @override a mixin's method will be limited to just that method, not effecting any of the other functions defined on the mixin.

However in the following snippet, notice insert (public method) calls setRange (public method) on line 531. As such @overrideing of setRange would effect insert.

void insert(int index, E element) {
checkNotNullable(index, "index");
var length = this.length;
RangeError.checkValueInInterval(index, 0, length, "index");
add(element);
if (index != length) {
setRange(index + 1, length + 1, this, index);
this[index] = element;
}
}

Also, some methods seem to be using the iterator for _ in loop. Like the insert method (example below, line 285) . While I prefer this syntax, is it in keeping with the description to read using only length and operator? Most of the other methods use index instead of iteration.

void addAll(Iterable<E> iterable) {
int i = this.length;
for (E element in iterable) {
assert(this.length == i || (throw ConcurrentModificationError(this)));
add(element);
i++;
}
}

I actually much prefer the looping using iterator, and prefer it if the methods were written using it's declarative style (as opposed to the imperative style required by limiting to using operator[] and length. But just want to make sure it's in keeping with the documentation.

@mit-mit
Copy link
Member

mit-mit commented Jan 31, 2023

Review expected in https://dart-review.googlesource.com/c/sdk/+/280075

@asashour
Copy link
Contributor

asashour commented Feb 14, 2023

@jarrodcolburn there are test failures, e.g. here.

Please update the expectation files in the PR.

Another way is to use Gerrit to propose changes as described here.

@asashour
Copy link
Contributor

https://dart-review.googlesource.com/c/sdk/+/283682

which includes additional places than the ones in the PR, and updates the front_end expectations.

copybara-service bot pushed a commit that referenced this pull request Apr 12, 2023
Fixes #51171

CoreLibraryReviewExempt: Removes no-longer-needed non-null promotion.
Change-Id: I46215a274c3042619578bf25bff045b15d4e087f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/283682
Reviewed-by: Lasse Nielsen <lrn@google.com>
Commit-Queue: Lasse Nielsen <lrn@google.com>
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.

4 participants