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

Initial rough support for deducing generic arguments in a call to a generic function. #4184

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Aug 2, 2024

No description provided.

Copy link
Contributor

@geoffromer geoffromer left a comment

Choose a reason for hiding this comment

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

FYI I haven't reviewed the generated SemIR yet; I'm looking for guidance about how best to do that.

Also FYI, I'm on vacation next week, so if we don't finish this today you may need to find another reviewer.

toolchain/check/deduce.cpp Outdated Show resolved Hide resolved
toolchain/check/deduce.cpp Show resolved Hide resolved
toolchain/check/deduce.cpp Outdated Show resolved Hide resolved
toolchain/check/deduce.cpp Outdated Show resolved Hide resolved
toolchain/check/deduce.cpp Show resolved Hide resolved
toolchain/check/deduce.cpp Outdated Show resolved Hide resolved
toolchain/check/deduce.cpp Show resolved Hide resolved
toolchain/check/testdata/function/generic/deduce.carbon Outdated Show resolved Hide resolved
Copy link
Contributor

@geoffromer geoffromer left a comment

Choose a reason for hiding this comment

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

(Finished initial review)

// CHECK:STDOUT: %U.ref.loc15: type = name_ref U, %U.loc15_10.2 [symbolic = @Get.%U.1 (constants.%U)]
// CHECK:STDOUT: %.loc15_28.1: %.3 = tuple_literal (%T.ref.loc15, %U.ref.loc15)
// CHECK:STDOUT: %.loc15_28.2: type = converted %.loc15_28.1, constants.%.4 [symbolic = @Get.%.1 (constants.%.4)]
// CHECK:STDOUT: %return.var.loc15: ref @Get.%.1 (%.4) = var <return slot>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this %.4 be constants.%.4? If not, I can't find what it's referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Names in SemIR formatted types are by default in the constants scope, because type constants are almost always in that scope. (This can be confusing, but it does substantially reduce the verbosity. Maybe we should reconsider, perhaps using a shorter name for constants?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion about how to spell names that refer to constants, but what really threw me off here was the fact that the previous line uses the longer form constants.%.4. Is there some significance to that? If not, it would be nice to fix the inconsistency one way or the other, but presumably that's out of scope for this PR.

@zygoloid zygoloid requested a review from geoffromer August 2, 2024 21:51
// CHECK:STDOUT: %U.ref.loc15: type = name_ref U, %U.loc15_10.2 [symbolic = @Get.%U.1 (constants.%U)]
// CHECK:STDOUT: %.loc15_28.1: %.3 = tuple_literal (%T.ref.loc15, %U.ref.loc15)
// CHECK:STDOUT: %.loc15_28.2: type = converted %.loc15_28.1, constants.%.4 [symbolic = @Get.%.1 (constants.%.4)]
// CHECK:STDOUT: %return.var.loc15: ref @Get.%.1 (%.4) = var <return slot>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion about how to spell names that refer to constants, but what really threw me off here was the fact that the previous line uses the longer form constants.%.4. Is there some significance to that? If not, it would be nice to fix the inconsistency one way or the other, but presumably that's out of scope for this PR.

@zygoloid zygoloid added this pull request to the merge queue Aug 2, 2024
Merged via the queue into carbon-language:trunk with commit b3fcaf9 Aug 2, 2024
7 checks passed
@zygoloid zygoloid deleted the toolchain-deduction branch August 2, 2024 22:58
brymer-meneses pushed a commit to brymer-meneses/carbon-lang that referenced this pull request Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants