Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #2355. Add more typed_data.setRange() tests #2412

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

sgrekhov
Copy link
Contributor

@sgrekhov sgrekhov commented Dec 5, 2023

No description provided.

@eernstg eernstg changed the title Fixes #2355. Add more typed_date.setRange() tests Fixes #2355. Add more typed_data.setRange() tests Dec 5, 2023
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

It looks good! However, I need to understand the test a little bit better.

In particular, I'm trying to understand how the cases in this test were selected, in order to be able to characterize the coverage. Here are the pairs of types that are tested against each other, with the expectation that setRange throws (the list has been sorted alphabetically):

Float32List - Int16List
Float32List - Int32List
Float32List - Int32x4List
Float32List - Int64List
Float32List - Int8List
Float32List - Uint16List
Float32List - Uint32List
Float32List - Uint64List
Float32List - Uint8ClampedList
Float32List - Uint8List
Float32x4List - Int32x4List
Float64List - Int32List
Float64List - Int64List
Float64List - Uint32List
Float64List - Uint64List
Float64x2List - Int64List
Float64x2List - Uint64List
Int16List - Float32x4List
Int32x4List - Float32x4List
Int32x4List - Float64List
Int32x4List - Float64x2List
Int64List - Float32x4List
Int64List - Float64x2List
Int64List - Uint32List
Uint16List - Float32List
Uint32List - Float32List
Uint32List - Float64List
Uint64List - Float32List
Uint64List - Float64List

So we have a different number of tests involving different types: 11 tests involving Float32List, 4 * Float32x4List, 7 * Float64List, 4 * Float64x2List, 2 * Int16List, 5 * Int32x4List, 6 * Int64List, 2 * Uint16List, 5 * Uint32List, 6 * Uint64List, 3 * Int32List, 1 * Int8List, 1 * Uint8List.

Perhaps the idea is that we're testing "each float type against each int type", but in that case we're missing 'Float64List - Int16List', 'Float64List - Uint16List', and a few others. (With 4 Float... types and 10 Int... and Uint... types, we'd have 40 combinations, but we have 29). Are there some combinations that are omitted because they aren't testable?

I'm also unsure about the x types (for example, Int32x4List and Float64x2List). They seem to be associated with a specialized usage of a bit vector containing 128 bits (as a specialized way to handle 2 64 bit registers as a single operand in an SIMD instruction, or something like that), so maybe they should just be considered to be values with the size 128 and a specialized type which is "4 32-bit ints", and hence different from a Int32List with 4 times as many elements. Or should I think about them in a different way?

@sgrekhov
Copy link
Contributor Author

sgrekhov commented Dec 6, 2023

The idea is to test each setRange() of int type against a float type of the same size (to ensure that now we have an error, not a memcopy), against a type of a smaller size and against a type of a bigger size. And the same for each float agains int. But we have float types with sizes 32, 64, and 128 (32x4 and 64x2) only. Therefore I had to test Int16 against Uint8 and Uint16 in case of small and equal size which is not an error. If so, let's also check then that Int16List.setRange() in not an error against Uint32 as well. But it is an error against Float32 in a separate test.

Also, when we are checking 32-bit type against 16, 32 and 64 bit, the latter case moved to a separate tests because it has to be switched off on Web.

Issue, described in dart-lang/sdk#53945 exists for x types as well, so I test x types too.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

OK, thanks a lot!

LGTM.

@eernstg
Copy link
Member

eernstg commented Dec 6, 2023

If so, let's also check then that Int16List.setRange() in not an error against Uint32 as well.

Very good -- I'll just land this PR, and then extra tests can be added as needed in a separate PR.

@eernstg eernstg merged commit af60144 into dart-lang:master Dec 6, 2023
2 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Dec 11, 2023
2023-12-07 sgrekhov22@gmail.com Fixes dart-lang/co19#2413. Add missing expected compile-time errors for CFE (dart-lang/co19#2418)
2023-12-07 sgrekhov22@gmail.com dart-lang/co19#2119. Run dart formatter on LibTest/async tests (dart-lang/co19#2417)
2023-12-06 sgrekhov22@gmail.com dart-lang/co19#2398. Make asyncStart/End() safe in LibTest/async (dart-lang/co19#2416)
2023-12-06 sgrekhov22@gmail.com Fixes dart-lang/co19#2355. Add more typed_date.setRange() tests (dart-lang/co19#2412)
2023-12-06 sgrekhov22@gmail.com dart-lang/co19#2404. Small code-style improvements and description update (dart-lang/co19#2414)
2023-12-04 sgrekhov22@gmail.com dart-lang/co19#2004. Add deferred libraries tests according to the changed spec (dart-lang/co19#2411)
2023-12-04 sgrekhov22@gmail.com Fixes dart-lang/co19#2383. Add more constant evaluation tests (dart-lang/co19#2396)

Change-Id: I15e0d681538fa0f2a311f74d1930fad7270b81a0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/340561
Commit-Queue: Alexander Thomas <athom@google.com>
Reviewed-by: Erik Ernst <eernst@google.com>
Reviewed-by: Alexander Thomas <athom@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.

None yet

2 participants