Fix Issue 3351: Shared discriminator having the same value< #3458
Conversation
|
redundant header include, i have noticed, will fix after review |
There was a problem hiding this comment.
Fantastic work i've left you comments for style. The error message using %< etc will fix the warnings that are introduced.
I don't think you should need the changes to rust-compile-item.cc but left me know if removing it causes an issue for you
I think in theory we might need a guard in your code where check_variant_record_collision needs to check if this has been checked before because you will get duplicate warnings here for example:
#[lang="sized"]
trait Sized {}
enum Greeting<T> {
Hi(T),
Bye
}
fn test() {
let a = Greeting::<i32>::Hi(123);
let b = Greeting::<f32>::Bye;
}
So I think lets get this first part merged. Then you will need to start tracking the HIR::Enum def-id on the TyTy::ADTType and you can keep a map in the backend if this DefID has already been checked to solve the above problem.
badumbatish
force-pushed
the
issue_3351
branch
3 times, most recently
from
c59acb0 to
08229bd
Compare
badumbatish
force-pushed
the
issue_3351
branch
from
08229bd to
b910edb
Compare
thank you for your guidance! |
|
|
||
| rich_location r (line_table, type.get_locus ()); | ||
| std::string assigned_here_msg | ||
| = "`" + std::to_string (discriminant_integer) + "`" + " assigned here"; |
There was a problem hiding this comment.
There are more backquote here, GCC probably does't complain about those because they are not in the format string. I'm unsure if we should change them.
There was a problem hiding this comment.
I can change it to %qlld via format string i think? The reason i did it this way is because different platform has different width for HOST WIDTH INT and im not sure if the final answer would be %qlld or %qld or if there is any complication if HWI is only %d and we use %qlld
There was a problem hiding this comment.
Yeah, I'm kinda torn on the subject. Raw back quotes don't play well with different locales, this is not only a matter of style.
There was a problem hiding this comment.
Would HOST_WIDE_INT_PRINT_DEC work?
There was a problem hiding this comment.
i think it could work for rust_error_at,
for add_fixit_replace() they dont take formatting arguments, is there a way to create a standalone formatting string in the codebase without explicitly using snprintf?
There was a problem hiding this comment.
Maybe use normal quotation marks (
") instead of`and include aTODOcomment, so we're more likely to notice and fix this in the future
i will opt for this. fix coming soon
There was a problem hiding this comment.
Actually, wait, we have rust_open_quote and rust_close_quote -- that should do the trick
There was a problem hiding this comment.
Oh, wait, check out expand_message in rust-diagnostics.cc -- that should be what you're looking for
There was a problem hiding this comment.
Oh, wait, check out
expand_messageinrust-diagnostics.cc-- that should be what you're looking for
the function is only in rust-diagnostics.cc, im not sure if i should move it to the header to use it solely for rust-compile-type.cc?
There was a problem hiding this comment.
You could make it not-static and add a declaration to rust-diagnostics.h
gcc/testsuite/ChangeLog: * rust/compile/shared_discriminator_enum.rs: New test.
gcc/rust/ChangeLog: * backend/rust-compile-item.cc (CompileItem::visit): Use HIR::Enum visit * backend/rust-compile-item.h: Fix clash of enum shared discriminant * backend/rust-compile-type.cc (check_variant_record_collision): Likewise. (TyTyResolveCompile::visit): Early return if fail enum check * rust-diagnostics.cc (expand_message): non-static now. * rust-diagnostics.h (RUST_ATTRIBUTE_GCC_DIAG): non-static expand msg (expand_message): likewise.
Fixes #3351
Overriding on the compile item visitor, I am not sure if this is the right way to go?