Skip to content

Fix narrowing conversion warnings#3732

Merged
CohenArthur merged 1 commit intoRust-GCC:masterfrom
powerboat9:fix-warn
Apr 28, 2025
Merged

Fix narrowing conversion warnings#3732
CohenArthur merged 1 commit intoRust-GCC:masterfrom
powerboat9:fix-warn

Conversation

@powerboat9
Copy link
Collaborator

@powerboat9 powerboat9 commented Apr 10, 2025

@powerboat9
Copy link
Collaborator Author

powerboat9 commented Apr 11, 2025

Open to suggestions on how to make this a bit less ugly, but we should probably fix this bug before GCC 15 releases

CohenArthur
CohenArthur requested changes Apr 11, 2025
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 don't think this is the best way to go about this. In some places the ID stored was size - 1, which is different here. I think we can also find a cleaner way to do this, like maybe storing size_t instead of uint32_t in all of those *Id structs? It used to be an issue when they were type aliases but that's no longer the case

@powerboat9
Copy link
Collaborator Author

powerboat9 commented Apr 11, 2025

The emplace_back calls increment the value returned by size. As for changing the value type to size_t, that does sound ideal, but I don't know why we're using uint32_t in the first place, some data structure size optimization?

@powerboat9
Copy link
Collaborator Author

powerboat9 commented Apr 11, 2025

@jdupak

@powerboat9
Copy link
Collaborator Author

powerboat9 commented Apr 11, 2025

I don't think this is the best way to go about this. In some places the ID stored was size - 1, which is different here. I think we can also find a cleaner way to do this, like maybe storing size_t instead of uint32_t in all of those *Id structs? It used to be an issue when they were type aliases but that's no longer the case

What was an issue with type aliases? Some 32-bit compatibility issue, where uint32_t may or may not have been equivalent to size_t? I think I remember something like that

@powerboat9
Fix narrowing conversion warnings
b85998f 
Fixes PR#119641

gcc/rust/ChangeLog:

	* checks/errors/borrowck/rust-bir-place.h
	(IndexVec::size_type): Add.
	(IndexVec::MAX_INDEX): Add.
	(IndexVec::size): Change the return type to the type of the
	internal value used by the index type.
	(PlaceDB::lookup_or_add_variable): Use the return value from the
	PlaceDB::add_place call.
	* checks/errors/borrowck/rust-bir.h
	(struct BasicBlockId): Move this definition before the
	definition of the struct Function.

Signed-off-by: Owen Avery <powerboat9.gamer@gmail.com>
@powerboat9
Copy link
Collaborator Author

powerboat9 commented Apr 17, 2025

I think I've found a better way to do this, by modifying IndexVec

@powerboat9 powerboat9 requested a review from CohenArthur April 17, 2025 04:36
CohenArthur
CohenArthur approved these changes Apr 22, 2025
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.

ooooh I really like that, well done!! I think this would be really good

@CohenArthur CohenArthur added this pull request to the merge queue Apr 28, 2025
Merged via the queue into Rust-GCC:master with commit 161e3c6 Apr 28, 2025
12 checks passed
@powerboat9 powerboat9 deleted the fix-warn branch April 28, 2025 21:10
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

@philberty philberty Awaiting requested review from philberty

@jdupak jdupak Awaiting requested review from jdupak

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@powerboat9 @CohenArthur