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

First stab at issue 2855 by spltting the two maps #2874

Merged
merged 1 commit into from
Mar 22, 2024
Merged

First stab at issue 2855 by spltting the two maps #2874

merged 1 commit into from
Mar 22, 2024

Conversation

badumbatish
Copy link
Contributor

@CohenArthur
Hi there, I'm looking for some review/feedback on this issue. I just finished splitting the BiMap and unordered_map of the MacroBuiltin class.

I'm planning on putting the non-member functions' declaration in gcc/rust/expand/rust-macro-builtins.h but ran into some error with the compiler not knowing Parser as a class, etc etc.

@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch 3 times, most recently from a1a8fa3 to f8758f1 Compare February 26, 2024 23:49
@badumbatish badumbatish marked this pull request as ready for review February 26, 2024 23:50
CohenArthur
CohenArthur reviewed Feb 27, 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.

what error did you run into specifically? I saw your comment about circular inclusion on Zulip, I think we should be able to find a solution

@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch from f8758f1 to 887105f Compare February 27, 2024 11:14
@badumbatish
Copy link
Contributor Author

what error did you run into specifically? I saw your comment about circular inclusion on Zulip, I think we should be able to find a solution

So initially i want to move the declaration of all non-member functions inside the header and add the necessary headers to make the declaration complete, one of the files was rust-parse.h. When i added it, it creates a circular dependency like this

rust-parse.h -> rust-item.h -> rust-expr.h -> rust-macro.h -> rust-macro--builtins.h -> rust-parse.h

I was a bit impatient and decided to put all the non-member functions declaration inside a new header to avoid this circular dependency.
The logs are in this chain of messages if you want to take a look. It's about 20000 lines long (jeez) https://gcc-rust.zulipchat.com/#narrow/stream/327528-GSoC/topic/issue.202855

@CohenArthur
Copy link
Member

what error did you run into specifically? I saw your comment about circular inclusion on Zulip, I think we should be able to find a solution

So initially i want to move the declaration of all non-member functions inside the header and add the necessary headers to make the declaration complete, one of the files was rust-parse.h. When i added it, it creates a circular dependency like this

rust-parse.h -> rust-item.h -> rust-expr.h -> rust-macro.h -> rust-macro--builtins.h -> rust-parse.h

I was a bit impatient and decided to put all the non-member functions declaration inside a new header to avoid this circular dependency. The logs are in this chain of messages if you want to take a look. It's about 20000 lines long (jeez) https://gcc-rust.zulipchat.com/#narrow/stream/327528-GSoC/topic/issue.202855

ah, I see! so I think one way to do it would indeed be to put the "parsing" function declarations in rust-macro-builtins.h. these functions should not have any structs/enums from rust-parse.h in their declarations, as they are supposed to create instances of parsers unless they are helper functions.

another possibility is to indeed split the parsing functions into their own header + source, which includes rust-parse.h but isn't included by rust-macro-builtins.h. then, the builtin macro handlers (in rust-macro-builtin-asm for example) can include that header to gain access to the parsing functions.

@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch 6 times, most recently from 259fbd2 to 225d64e Compare February 28, 2024 04:39
@badumbatish
Copy link
Contributor Author

i must have missed these errors on my local build, i'll try running the same command as via github actions

@badumbatish
Copy link
Contributor Author

accidently clicked on merge from master, will revisit and sort this out tmr

@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch from 86972bc to 741bc67 Compare February 29, 2024 00:03
@CohenArthur
Copy link
Member

@badumbatish is this ready for review? if not, could you put it in Draft please? :) thanks

@badumbatish
Copy link
Contributor Author

badumbatish commented Mar 4, 2024 •
edited
Loading

@badumbatish is this ready for review? if not, could you put it in Draft please? :) thanks

hi! it is ready for review, i'm not sure if i should resolve the conflict from master or wait for the review and then resolve it?

dkm
dkm reviewed Mar 5, 2024
View reviewed changes
Copy link
Member

@dkm dkm left a comment

Choose a reason for hiding this comment

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

minor remarks. I'll let the real devs do a real review :)

@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch 2 times, most recently from 9eda59f to bbd3713 Compare March 7, 2024 00:14
@badumbatish badumbatish marked this pull request as draft March 7, 2024 00:28
@badumbatish
Copy link
Contributor Author

Not ready for review yet there seems to be some problem with rebasing

@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch 3 times, most recently from 38c1a33 to c916dd2 Compare March 7, 2024 02:09
@badumbatish badumbatish marked this pull request as ready for review March 7, 2024 05:03
@badumbatish
Copy link
Contributor Author

@CohenArthur hi Arthur, I just rebased the master branch into this branch and its ready for review.

CohenArthur
CohenArthur reviewed Mar 7, 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.

thanks for working on this :) I think only a couple nitpicks and then we can merge it!

@badumbatish
Copy link
Contributor Author

@CohenArthur everything should be good to go

@badumbatish badumbatish mentioned this pull request Mar 9, 2024
Closed
CohenArthur
CohenArthur approved these changes Mar 19, 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.

last nitpick and we'll merge this (sorry :D)

@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch 2 times, most recently from e0e7a6b to cf58569 Compare March 20, 2024 05:58
@CohenArthur CohenArthur self-assigned this Mar 21, 2024
@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch from cf58569 to 3ab154b Compare March 21, 2024 20:04
@badumbatish
Split up rust-macro-builtins.cc
0dc7375 
Fixes issue Rust-GCC#2855

gcc/rust/ChangeLog:

	* Make-lang.in: add new .o builds for new .cc files
	* expand/rust-cfg-strip.h (RUST_CFG_STRIP_H): Add include guards
	for rust-cfg-strip
	* expand/rust-macro-builtins.cc (make_macro_path_str): moved to new respective files
	(make_token): moved to new respective files
	(make_string): moved to new respective files
	(macro_end_token): moved to new respective files
	(try_extract_string_literal_from_fragment): moved to new respective files
	(try_expand_many_expr): moved to new respective files
	(parse_single_string_literal): moved to new respective files
	(source_relative_path): moved to new respective files
	(load_file_bytes): moved to new respective files
	(MacroBuiltin::assert_handler): moved to new respective files
	(MacroBuiltin::file_handler): moved to new respective files
	(MacroBuiltin::column_handler): moved to new respective files
	(MacroBuiltin::include_bytes_handler): moved to new respective files
	(MacroBuiltin::include_str_handler): moved to new respective files
	(MacroBuiltin::compile_error_handler): moved to new respective files
	(MacroBuiltin::concat_handler): moved to new respective files
	(MacroBuiltin::env_handler): moved to new respective files
	(MacroBuiltin::cfg_handler): moved to new respective files
	(MacroBuiltin::include_handler): moved to new respective files
	(MacroBuiltin::line_handler): moved to new respective files
	(MacroBuiltin::stringify_handler): moved to new respective files
	(struct FormatArgsInput): moved to new respective files
	(struct FormatArgsParseError): moved to new respective files
	(format_args_parse_arguments): moved to new respective files
	(MacroBuiltin::format_args_handler): moved to new respective files
	* expand/rust-macro-builtins.h (builtin_macro_from_string):
	merge tl::optional from master
	* expand/rust-macro-builtins-asm.cc: New file.
	* expand/rust-macro-builtins-format-args.cc: New file.
	* expand/rust-macro-builtins-helpers.cc: New file.
	* expand/rust-macro-builtins-helpers.h: New file.
	* expand/rust-macro-builtins-include.cc: New file.
	* expand/rust-macro-builtins-location.cc: New file.
	* expand/rust-macro-builtins-log-debug.cc: New file.
	* expand/rust-macro-builtins-test-bench.cc: New file.
	* expand/rust-macro-builtins-trait.cc: New file.
	* expand/rust-macro-builtins-utility.cc: New file.
@badumbatish badumbatish force-pushed the split_up_rust_macro_builtsin branch from 0307e01 to 0dc7375 Compare March 21, 2024 20:29
@CohenArthur CohenArthur added this pull request to the merge queue Mar 21, 2024
Merged via the queue into Rust-GCC:master with commit 2f334bb Mar 22, 2024
9 checks passed
@CohenArthur CohenArthur mentioned this pull request Mar 27, 2024
Closed
@badumbatish badumbatish deleted the split_up_rust_macro_builtsin branch May 18, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkm dkm dkm left review comments

@CohenArthur CohenArthur CohenArthur approved these changes

Assignees

@CohenArthur CohenArthur

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@badumbatish @CohenArthur @dkm