Update toolchain/check/testdata/interop/cpp/function_param_int*.carbon and toolchain/check/testdata/interop/cpp/function_return.carbon tests to use sem ir ranges
#5645
Conversation
…on` and `toolchain/check/testdata/interop/cpp/function_return.carbon` tests to use sem ir ranges
…mIR for cases intended to fail. Keep one successful case with full SemIR.
| //@dump-sem-ir-begin | ||
| //@dump-sem-ir-end |
There was a problem hiding this comment.
Why not use ranges=only? If there are tests that you're intentionally trying to get full SemIR for, can you perhaps split them out to a separate file to make it more apparent?
There was a problem hiding this comment.
+1 for moving the Full SemIR tests in a separate file.
That will make it easier to adapt that test only for the purpose of testing the SemIR output and make it more apparent what we want to have there. For example we can also skip the int param test and keep only one for the params. I'd still keep existing tests as they are and make them dedicated to type mapping.
There was a problem hiding this comment.
How about having only one file for the Full SemIR tests? (e.g. function_semir.carbon)
I think that in general we don't need a full SemIR test for each of the param types, only one is enough (similar to what you do for the return types). I'd also add a Carbon function with the same signature there, to be able to compare the SemIR for the C++ and Carbon functions.
There was a problem hiding this comment.
Done.
I've kept full_semir in the filename though, since other files also have SemIR but this file has the full SemIR so I thought it would be clearer.
Let me know if you want me to remove the full_ part :)
There was a problem hiding this comment.
FWIW, there's a "function" subdirectory, you could consider moving it there to be `function/full_semir.carbon" instead of "function_full_semir.carbon". Similar with params. But, this is an idle comment -- I'm happy with the PR as-is, just can't merge it due to conflicts.
There was a problem hiding this comment.
Thanks!
I think I'll move files to under function dir in a separate PR.
This is a follow up of not having the no_prelude dir.
ivanaivanovska
left a comment
ivanaivanovska
left a comment
There was a problem hiding this comment.
How about having only one file for the Full SemIR tests? (e.g. function_semir.carbon)
I think that in general we don't need a full SemIR test for each of the param types, only one is enough (similar to what you do for the return types). I'd also add a Carbon function with the same signature there, to be able to compare the SemIR for the C++ and Carbon functions.
…unction_` prefix Follow up of carbon-language#5645 (comment) and carbon-language#5607.
…unction_` prefix (#5716) Follow up of #5645 (comment) and #5607.
3 participants
Follow up of #5594.
Trying to compromise SemIR size, having enough information and complexity of tests, I've duplicated representative tests to a separate test file with
--dump-sem-ir-ranges=if-present.