fix Issue 17843 - -betterC struct with field generates references to…

[WalterBright]

… TypeInfo

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Description
✓	17843	-betterC struct with double field generates references to TypeInfo

[MartinNowak]

That test file better be named betterc.d ;).

[WalterBright]

I use grep to search for relevant test cases anyway. The filenames of the test cases aren't particularly helpful.

[WalterBright]

renamed

[MartinNowak]

If any of the fields has an opEquals, then we need it too.

Sounds similarly problematic, wouldn't it be better to just disable the toHash altogether? The runtime based AA is out of reach for -betterC anyhow.

[WalterBright]

I went for disabling as little as possible.

[dnadlinger]

Can we please make sure -betterC makes unsupported things fail visibly (ie. compiler/linker error), and produces warnings where it changes behaviour? A few more changes like this where existing semantics are modified silently, and -betterC will have to be treated as an incompatible and woefully underdocumented language fork.

A large part of the appeal of -betterC is to be able to draw on existing D code when targeting constrained environments. As it stands, this change requires users to audit any piece of code for comparisons of structs that (transitively) contain floating point numbers.

[dnadlinger]

(Imho the correct fix would involve also detecting and disallowing such comparisons.)

[WalterBright]

toHash is only really of use for associative arrays, and those won't work with -betterC anyway (i.e. they won't link).

[dnadlinger]

Ah, sorry – somehow I seemed to remember that this was used for opEquals as well, but that it is indeed based on the separate needOpEquals.

The change should be okay, then. But as for disabling as little as possible vs. not emitting toHash altogether: Wouldn't the same problem also occur e.g. with array members?

[WalterBright]

A large part of the appeal of -betterC is to be able to draw on existing D code when targeting constrained environments.

I think that is a bit of a misunderstanding. betterC code does not link to Phobos or druntime at all, and very little existing D code will work that way unless it is specifically crafted that way.

betterC is a tool to enable D code to be written with zero overhead, so that there's no longer any reason to use C. It can also be used to gradually convert existing C code to D. I'm doing that now with the DMC++ compiler, which is how I ran into this PR issue.

[WalterBright]

array members?

Yes. Fixed. Good catch!

[MartinNowak]

Sorry, I still don't follow. You're modifying the function needToHash which is used to decide whether or not TypeInfo gets an auto-generated xtoHash function.
Without TypeInfo those xtoHash/xopEquals/xopCmp functions are not accessible anyhow.
Seems much smarter to just disable the auto-generation instead.

dmd/src/ddmd/dsymbolsem.d

Lines 5425 to 5427 in 2412363

	sd.xeq = buildXopEquals(sd, sc2);
	sd.xcmp = buildXopCmp(sd, sc2);
	sd.xhash = buildXtoHash(sd, sc2);

Furthermore you want to split the monolithic betterC switch into disableTypeInfo, disableEH, disableX right away, so we don't have to unravel that later on.

[WalterBright]

@MartinNowak I disabled the 3 functions from being generated with -betterC, but splitting the flag into multiple ones seems a bit excessive at the moment. Catching what it the flag does is pretty simple, grepping it is just a few hits. Splitting it into multiple ones means one would have to do multiple greps to see what the switch does.

[andralex]

I'm good with this. @MartinNowak ?

[andralex]

cc @MartinNowak

[JinShil]

Due to #7288, I think this should be checking the useTypeInfo flag.

[WalterBright]

done

[MartinNowak]

The splitting of the betterC options was obviously coming ;), glad it's in now.

[ibuclaw]

Yes, I introduced that. Please continue to enforce. :-)