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

Add more Dump methods for debugging #4747

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

josh11b
Copy link
Contributor

@josh11b josh11b commented Dec 27, 2024

Follow-on to #4669 . Did some manual testing, but may have not exercised all code paths.

@github-actions github-actions bot requested a review from jonmeow December 27, 2024 23:07
@josh11b josh11b requested review from danakj and removed request for jonmeow December 27, 2024 23:07
Copy link
Contributor

@danakj danakj left a comment

Choose a reason for hiding this comment

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

Cool thank you! A couple comments to make things more consistent with what's been done earlier.

}

LLVM_DUMP_METHOD auto Dump(const File& file, ClassId class_id) -> void {
llvm::errs() << class_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

The idea with the NoNewlines is that all the dumping logic lives in those functions so that they can be composed into other DumpNoNewline classes, and the body of each Dump() is just a DumpNoNewline() + newline. So this body here could move into a DumpNoNewline function.

Same for the other Dump functions below.

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 recognized that was the intent, and that is how I started writing this PR, but in practice it didn't seem to work out that way. The information I wanted to print for a given id (a) benefited from formatting using newlines and indents and (b) didn't decompose into calls to DumpNoNewline functions. To avoid extra boilerplate, I've instead only written DumpNoNewline functions when there was a caller that wanted the functionality of one of the Dump functions without the newline, instead of writing an extra function proactively.


namespace Carbon::SemIR {

static auto DumpIfValid(const File& file, NameId name_id) -> void {
Copy link
Contributor

Choose a reason for hiding this comment

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

DumpNameIfValid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


LLVM_DUMP_METHOD auto Dump(const File& file, EntityNameId entity_name_id)
-> void {
llvm::errs() << entity_name_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest for each of these to write the type being dumped in the output too, see for example https://github.com/carbon-language/carbon-lang/pull/4747/files?diff=unified&w=0#diff-e690edf9337d3bd7e874588bc9d865536d6c69f2b7e8db79a9ef3aa9fd4b6edbL33-L42

So it could be something like EntityNameId(...the stuff in here...)

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 believe the print functions for each of the Id classes already includes its type:

out << IdT::Label;

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