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

Expand handling of function redeclarations. #3739

Merged
merged 8 commits into from
Mar 6, 2024

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Mar 4, 2024

This detects more cases of incorrect matches between declaration and definition. It also factors the logic out to a separate file for easier sharing, particularly when it comes to merging imports (I'm not sure if this exact API will be reshared, but the core logic should apply).

@jonmeow jonmeow force-pushed the fn-redecl branch 2 times, most recently from 848f3db to 55edf85 Compare March 5, 2024 18:47
toolchain/check/function.cpp Outdated Show resolved Hide resolved
toolchain/check/function.cpp Outdated Show resolved Hide resolved
}

// Returns true if two param refs agree.
static auto DoParamAgrees(Context& context, SemIR::InstId new_decl_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static auto DoParamAgrees(Context& context, SemIR::InstId new_decl_id,
static auto DoParamsAgree(Context& context, SemIR::InstId new_decl_id,

I think? (I'd read "DoParamAgrees" as "do the 'param-agrees' process", which might be accurate but seems a bit awkward.)

But given that this is producing diagnostics rather than just computing a bool, I think naming this so it doesn't sound like a predicate might be better. Maybe:

Suggested change
static auto DoParamAgrees(Context& context, SemIR::InstId new_decl_id,
static auto CheckParamsAgree(Context& context, SemIR::InstId new_decl_id,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was some sloppy naming, but how about just CheckRedecl + Param + s as naming. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That name works for me, but I think it'd be better to have more difference between the names of CheckRedeclParams and CheckRedeclParam to reflect that one of them is producing diagnostics and the other is just performing a comparison. (Or maybe make them both produce diagnostics per the FIXME.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved (not really addressing the FIXME, I don't think the structure is worth addressing)

toolchain/check/function.cpp Outdated Show resolved Hide resolved
toolchain/check/function.cpp Outdated Show resolved Hide resolved
@jonmeow jonmeow force-pushed the fn-redecl branch 2 times, most recently from d4f89ac to fac1185 Compare March 6, 2024 00:58
@zygoloid zygoloid enabled auto-merge March 6, 2024 20:12
@zygoloid zygoloid added this pull request to the merge queue Mar 6, 2024
Merged via the queue into carbon-language:trunk with commit 1bf9b24 Mar 6, 2024
7 checks passed
@jonmeow jonmeow deleted the fn-redecl branch March 6, 2024 21:59
@jonmeow jonmeow mentioned this pull request Mar 11, 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