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.

Sign up for GitHub

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

Jump to bottom

an attempt to implement pretty HIR dump #1933

Merged
merged 1 commit into from
Mar 2, 2023
Merged

an attempt to implement pretty HIR dump #1933

merged 1 commit into from
Mar 2, 2023

Conversation

00AR
Copy link
Contributor

@00AR 00AR commented Feb 27, 2023

In this draft, I have attempted to implement some part of pretty HIR Dump to show the proposed format for HIR Dump. My approach in this is to avoid printing any empty fields of HIR nodes. In the pretty HIR dump of the code example given below, no attributes are present in the code for the crate, so you will not see inner attribute: [] at all. This makes the dump look cleaner as only the available information is printed.

Try this example for the dump

fn main() {
    let var1=5;
}

fn test_1(arg1:u32, arg2:f32)->u8 {
    let new1:u8 = 4;
    let new2:u8 = 6 + 5;
    new2
}

fn test(mul:u32)->u32 {
    let first = 5;
    let second = first + 5;
    second
}

Seeking feedback for making it better :)
I will be making a GSOC proposal for this project.

@00AR 00AR marked this pull request as draft February 27, 2023 12:45
@CohenArthur CohenArthur added this to the Final upstream patches milestone Feb 27, 2023
CohenArthur
CohenArthur reviewed Feb 27, 2023
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

This is good work, thank you for working on this :) I like the idea of not having empty vectors everywhere. I think for your proposal it might be nice to consider a new side of the HIR dump which would be a more configurable dump - so for example, it might be nice to have an option to disable the printing of HirIds or NodIds. They are useful but might clutter the output if one is not looking for them. I'm looking forward to your proposal!

gcc/rust/hir/rust-hir-dump.cc Outdated
Comment on lines 37 to 40
stream << "inner_attrs"
<< ":"
<< " "
<< "[";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stream << "inner_attrs"
<< ":"
<< " "
<< "[";
stream << "inner_attrs: ["

gcc/rust/hir/rust-hir-dump.cc Outdated
Comment on lines 43 to 44
stream << "]"
<< "," << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
stream << "]"
<< "," << std::endl;
stream << "]," << std::endl;

gcc/rust/hir/rust-hir-dump.cc Outdated
indent++;
// TODO: outer attributes
stream << std::string (indent, indent_char);
stream << "let ";
Copy link
Member

Choose a reason for hiding this comment

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

Probably clearer to have something like LetStmt: like we have for Stmt: , Expr: , Function: etc

@00AR
Copy link
Contributor Author

00AR commented Feb 27, 2023

Thanks for reviewing!

I think for your proposal it might be nice to consider a new side of the HIR dump which would be a more configurable dump - so for example, it might be nice to have an option to disable the printing of HirIds or NodIds.

If I understand correctly, you mean introduce some more options other than -frust-dump-hir-pretty, such as -frust-dump-hir-pretty-no-hid and -frust-dump-hir-pretty-no-nid.

@CohenArthur
Copy link
Member

Thanks for reviewing!

I think for your proposal it might be nice to consider a new side of the HIR dump which would be a more configurable dump - so for example, it might be nice to have an option to disable the printing of HirIds or NodIds.

If I understand correctly, you mean introduce some more options other than -frust-dump-hir-pretty, such as -frust-dump-hir-pretty-no-hid and -frust-dump-hir-pretty-no-nid.

Yes :) but we can see about this later down the line :) I think this PR is in a good enough state already if you fix the few suggestions I left, and we can merge it :)

@00AR 00AR force-pushed the hir_dump branch 3 times, most recently from 114584f to c532055 Compare March 2, 2023 08:42
CohenArthur
CohenArthur approved these changes Mar 2, 2023
View reviewed changes
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

Great stuff :) is this still a draft or are you happy enough for us to merge it?

@00AR 00AR marked this pull request as ready for review March 2, 2023 09:21
@00AR
Copy link
Contributor Author

00AR commented Mar 2, 2023

are you happy enough for us to merge it?

Yes, I was just waiting for the checks to pass.
Thank you for the review!

@00AR @CohenArthur
added support for printing HIR dump of functions, statements, arithem…
6738f13 
…atic/logical expressions and literals.

gcc/rust/ChangeLog:

	* hir/rust-hir-dump.cc (Dump::go): support inner attrs, crate items and node mappings
	(Dump::visit): support functions, arith/logical exprs, let stmts and literals

Signed-off-by: Abdul Rafey <abdulrafeyq@gmail.com>
@CohenArthur CohenArthur added this pull request to the merge queue Mar 2, 2023
Merged via the queue into Rust-GCC:master with commit 724d3e8 Mar 2, 2023
@00AR 00AR deleted the hir_dump branch April 6, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CohenArthur CohenArthur CohenArthur approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
Final upstream patches
Development

Successfully merging this pull request may close these issues.
2 participants
@00AR @CohenArthur