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

Infer crate name after file opening #3146

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

Conversation

dylngg
Copy link

@dylngg dylngg commented Aug 29, 2024

gcc/rust/ChangeLog:

* rust-session-manager.cc (Session::handle_crate_name): Remove                    
  crate name inference                                                              
  (Session::compile_crate): Add crate name inference and error if                   
  inferred name is empty. Remove CompileOptions::get_instance ()                    
  that returned a local copy of the options. Rename                                 
  crate_name_changed to crate_name_found to match semantics.                        
  (rust_crate_name_validation_test): Test inferring ".rs" name                      
* rust-session-manager.h: Modify handle_crate_name definition to                  
  include filename.

Fixes #3129. Also fixes an issue where filenames such as ".rs" provided cause an ICE.

Note that I didn't make a test because I wasn't sure how to use DejaGnu to test that a directory was provided instead of a file (because if I created a test file, then it wouldn't be a directory!). I could've made a ".rs" test file to test the empty crate name behavior, but then the test would be hidden...

I did manually run the following tests:

workspace@ee59f2d37013:~/gccrs/build$ ./gcc/crab1 -frust-incomplete-and-experimental-compiler-do-not-use .rs
crab1: error: crate name from filename .rs is empty

workspace@ee59f2d37013:~/gccrs/build$ ./gcc/crab1 -frust-incomplete-and-experimental-compiler-do-not-use gcc/
crab1: error: cannot open filename gcc/: Is a directory

workspace@ee59f2d37013:~/gccrs/build$ ./gcc/crab1 -frust-incomplete-and-experimental-compiler-do-not-use ~/
crab1: error: cannot open filename /home/workspace/: Is a directory

workspace@ee59f2d37013:~/gccrs/build$ ./gcc/crab1 -frust-incomplete-and-experimental-compiler-do-not-use .rs.txt
crab1: error: invalid character ‘.’ in crate name: ‘.rs’
.rs.txt:1: note: crate name inferred from this file

workspace@ee59f2d37013:~/gccrs/build$ ./gcc/crab1 -frust-incomplete-and-experimental-compiler-do-not-use missing.rs
crab1: error: cannot open filename missing.rs: No such file or directory

Finally, I am waiting for confirmation from my employer that I own the copyright to these changes. This may take a while. I will add a DCO once I get that confirmation.

@dylngg
Copy link
Author

dylngg commented Aug 29, 2024

Decided that for consistency, the ".rs" test case should follow the same output as ".rs.txt" and other invalid inferences. For example, instead of:

workspace@ee59f2d37013:~/gccrs/build$ ./gcc/crab1 -frust-incomplete-and-experimental-compiler-do-not-use .rs
crab1: error: crate name from filename .rs is empty

we should have:

workspace@ee59f2d37013:~/gccrs/build$ ./gcc/crab1 -frust-incomplete-and-experimental-compiler-do-not-use .rs
crab1: error: crate name is empty
.rs:1: note: crate name inferred from this file

Fixes Rust-GCC#3129.

gcc/rust/ChangeLog:

	* rust-session-manager.cc (Session::handle_crate_name): Remove
	crate name inference
	(Session::compile_crate): Add crate name inference and error if
	inferred name is empty. Remove CompileOptions::get_instance ()
	that returned a local copy of the options. Rename
	crate_name_changed to crate_name_found to match semantics.
	(rust_crate_name_validation_test): Test inferring ".rs" name
	* rust-session-manager.h: Modify handle_crate_name definition to
	include filename.
Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

Looks good, I wonder if there's a way for us to test this behavior using dejagnu easily (maybe @tschwinge knows a way ?).

@P-E-P
Copy link
Member

P-E-P commented Sep 11, 2024

@dylngg Once you get confirmation from your employer, feel free to ping me and add a message in this PR stating that you've read and understand how DCO works. I'll be more than happy to merge this work!

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.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE when input file name is a directory ending with slash
3 participants