Merge i16 and i32 param and return test files
#5760
Conversation
…` to `int32.carbon`
…` test files and rename param test files appropriately.
| // CHECK:STDERR: Cpp.foo(&a); | ||
| // CHECK:STDERR: ^~~~~~~ | ||
| // CHECK:STDERR: | ||
| Cpp.foo(&a); | ||
| //@dump-sem-ir-end | ||
| } | ||
|
|
||
| // CHECK:STDOUT: --- import_short.carbon | ||
| // ============================================================================ | ||
| // short return |
There was a problem hiding this comment.
How about moving the return tests up, just after the first param test? E.g. have a short_param test, then short_return test. After that we don't need to have param in the name of each test, as the tests are not about testing param or return, but testing the type.
There was a problem hiding this comment.
The short param section includes multiple tests so putting the return test after it seems a bit arbitrary.
The other param tests also test pointers and references, which we might want to also test with return values at some point, so I prefer to be explicit about the use case being tested.
I'm not sure we want interleaving param and return tests, so it makes sense to me to put all return tests after all param tests.
|
In general it looks good to merge these files, with a small remark that I left. One question: What are the files function_param_int16.carbon and function_param_int32.carbon? Shall they be deleted? |
Yes, I've noticed that yesterday as well. I guess some rename I made didn't delete the files. Will send a fix. |
See #5768. |
| @@ -6,115 +6,115 @@ | |||
| // | |||
| // AUTOUPDATE | |||
| // TIP: To test this file alone, run: | |||
| // TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/function/param_int16.carbon | |||
| // TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/function/int16.carbon | |||
There was a problem hiding this comment.
From discussion about thunking, we expect a lot of types to behave in a similar way:
- i32 and i64 should work without a thunk
- i8, i16, and i128 should require a thunk
- Similar for unsigned
I'm hesitant to get into a pattern where each supported type has its own file (e.g., int8.carbon, int64.carbon, int128.carbon, double.carbon, even the float.carbon I just mentioned; is there also char.carbon, etc?). If your renaming, perhaps filenames should be closer to categories of behavior, rather than individual types?
There was a problem hiding this comment.
I've renamed these to arithmetic_mapped and arithmetic_bridged, assuming we want a single test file that includes i32 and i64 and a different test file that includes the other arithmetic types, which require thunks.
Null pointers, which are fundamental but not arithmetic, can probably have their own test file.
We'll probably need to take out pointer and reference type tests out of these test files eventually, as they test quite a different use case.
There was a problem hiding this comment.
"Arithmetic" alone would mean +-*/; how about numbers or arithmetic_types? cppreference says arithmetic types, which gives weight to that option.
Also, why is float still separate in return.carbon?
There was a problem hiding this comment.
Thanks, makes sense.
Done.
This is instead of having one test file that has different return types and two test files for
i16andi32param tests, which doesn't seem consistent.First commit does file renames for easier review.
Part of #5063.