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

Cleanup macro invoc #2981

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cleanup macro invoc #2981

wants to merge 1 commit into from

Conversation

badumbatish
Copy link
Contributor

@badumbatish badumbatish commented May 8, 2024 •
edited
Loading

MacroInvoc Refactor for #2919

Remove get_macro_node_id in favor of get_node_id in MacroInvocation
Add ExteralItem node_id constructor to avoid extra get_node_id()

gcc/rust/ChangeLog:

        * ast/rust-macro.h: Likewise.
        * resolve/rust-early-name-resolver.cc (EarlyNameResolver::visit): Likewise.
        * util/rust-hir-map.cc (Mappings::insert_macro_invocation): Likewise.
        (Mappings::lookup_macro_invocation): Likewise.
        (Mappings::insert_bang_proc_macro_invocation): Likewise.

@badumbatish
Copy link
Contributor Author

badumbatish commented May 8, 2024 •
edited
Loading

ok i appear to have developed this from another branch other than master, fixing it rn

@badumbatish
MacroInvoc Refactor
8cf484d 
Remove get_macro_node_id in favor of get_node_id in MacroInvocation
Add ExteralItem node_id constructor to avoid extra get_node_id()

gcc/rust/ChangeLog:

	* ast/rust-macro.h: Likewise.
	* resolve/rust-early-name-resolver.cc (EarlyNameResolver::visit): Likewise.
	* util/rust-hir-map.cc (Mappings::insert_macro_invocation): Likewise.
	(Mappings::lookup_macro_invocation): Likewise.
	(Mappings::insert_bang_proc_macro_invocation): Likewise.
	(Mappings::lookup_bang_proc_macro_invocation): Likewise.
@badumbatish
Copy link
Contributor Author

badumbatish commented May 8, 2024 •
edited
Loading

@CohenArthur hi Arthur i'm not sure for things like node_id and locus, if you want to not explicitly declare them in MacroInvocation and just get_node_id and get_locus from the parent's class or if you just want to do them explicitly like what I've kept in the modification?

Also not sure what else needs to be clean up for this class..

CohenArthur
CohenArthur reviewed May 24, 2024
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.

I think this is already a good cleanup - I need to have a think about how exactly we can take it further. I'll come back to this later :) thank you!

gcc/rust/resolve/rust-early-name-resolver.cc
Comment on lines 468 to 471
if (has_semicolon)
source_node = invoc.get_macro_node_id ();
source_node = invoc.get_node_id ();
else
source_node = invoc.get_node_id ();
Copy link
Member

Choose a reason for hiding this comment

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

see for example here we're no longer doing anything different if the macro has a semicolon or not, so we can remove the if block

Copy link
Contributor Author

@badumbatish badumbatish May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

wow no idea how i missed this, thanks for the review, i'll resolve it along with new thoughts on this

@P-E-P
Copy link
Member

P-E-P commented Sep 10, 2024

I don't get your changelog message:

        * ast/rust-macro.h: Likewise.
        * resolve/rust-early-name-resolver.cc (EarlyNameResolver::visit): Likewise.
        * util/rust-hir-map.cc (Mappings::insert_macro_invocation): Likewise.
        (Mappings::lookup_macro_invocation): Likewise.
        (Mappings::insert_bang_proc_macro_invocation): Likewise.

There is nothing to be like, how could the first statement be likewise ? In case you meant "Like stated in the commit message" I would suggest duplicating the message because IIRC changelog may be cut from the commit message to be put in the changelog file.

@badumbatish
Copy link
Contributor Author

I don't get your changelog message:

        * ast/rust-macro.h: Likewise.
        * resolve/rust-early-name-resolver.cc (EarlyNameResolver::visit): Likewise.
        * util/rust-hir-map.cc (Mappings::insert_macro_invocation): Likewise.
        (Mappings::lookup_macro_invocation): Likewise.
        (Mappings::insert_bang_proc_macro_invocation): Likewise.

There is nothing to be like, how could the first statement be likewise ? In case you meant "Like stated in the commit message" I would suggest duplicating the message because IIRC changelog may be cut from the commit message to be put in the changelog file.

Purely an oversight from my end. I'll update it once the PR needs work again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CohenArthur CohenArthur CohenArthur left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@badumbatish @P-E-P @CohenArthur