Skip to content
New issue

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

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

Already on GitHub? # to your account

Add comments for instructions that lack it. #4112

Merged
merged 8 commits into from
Jul 9, 2024

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Jul 8, 2024

No description provided.

@jonmeow jonmeow requested a review from zygoloid July 8, 2024 23:57
@github-actions github-actions bot requested a review from josh11b July 8, 2024 23:57
@@ -354,6 +372,7 @@ struct BranchWithArg {
InstId arg_id;
};

// A builtin instruction. These are special and listed in `builtin_kind.def`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an instruction that represents a built-in function? Or invokes a built-in function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little confused by your question, builtin_kind.def doesn't contain any functions (right now, at least). Perhaps you opened up builtin_function_kind.def instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it'd be worth mentioning here that these are all builtin types specifically, other than <error> which can also be used as a non-type value.

Copy link
Contributor Author

@jonmeow jonmeow Jul 9, 2024

Choose a reason for hiding this comment

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

I've added an example. I really don't want to get into documenting what's already documented in builtin_kind.def though: to me, it's opening the door for skew because this doesn't own the contents of the enum.

struct SpliceBlock {
// TODO: Can we make Parse::NodeId more specific?
// TODO: Make Parse::NodeId more specific.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was an honest question -- it seems plausible that splice blocks could appear in pretty arbitrary places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed; I'd just deleted the comment, and then copy-pasted from the wrong spot.

@@ -354,6 +372,7 @@ struct BranchWithArg {
InstId arg_id;
};

// A builtin instruction. These are special and listed in `builtin_kind.def`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it'd be worth mentioning here that these are all builtin types specifically, other than <error> which can also be used as a non-type value.

toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved
toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved
toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved
toolchain/sem_ir/typed_insts.h Outdated Show resolved Hide resolved
jonmeow and others added 8 commits July 9, 2024 13:05
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
@jonmeow jonmeow force-pushed the inst-comments branch 2 times, most recently from 5cff002 to 894883a Compare July 9, 2024 20:09
@zygoloid zygoloid added this pull request to the merge queue Jul 9, 2024
Merged via the queue into carbon-language:trunk with commit cb674a1 Jul 9, 2024
7 checks passed
@jonmeow jonmeow deleted the inst-comments branch July 9, 2024 21:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants