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

fix Issue 23658 - .di generation of variables should turn them into d… #14851

Merged
merged 1 commit into from Jan 31, 2023

Conversation

WalterBright
Copy link
Member

…eclarations

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
23658 normal .di generation of variables should turn them into declarations

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14851"

@WalterBright
Copy link
Member Author

ping @rainers

@rainers
Copy link
Member

rainers commented Jan 28, 2023

Looks good for regular variables. For export variables, it might change the interpretation from dllexport to dllimport, most of the time to the better. But it could theoretically break linking to a static library (used via di files) that actually wants to export that variable from the final DLL. But I doubt that such a library exists.

You could also declare variables with initializer and inferred type extern by emitting them as extern typeof(initializer) var;.

Test coverage seems a bit low with just a single variable, and this one now has a rather odd combination of static and extern. So I'd suggest adding a couple more declarations, including export.

@rikkimax
Copy link
Contributor

So I just received your email @WalterBright, it ended up in my Spam folder.

I haven't got anything to add to Rainer's comment except to say I really want to leave .di generator until after redesigning export, its current model does not fit Windows. If you'd like to talk about why it's required I'm on BeerConf for the next couple of hours.

@WalterBright
Copy link
Member Author

@rainers for the static library thing, the user can always craft the .di file as he sees fit.

I like your typeof suggestion, I didn't think about that.

I'm not quite sure what to do with static mixed with extern. static is pretty much ignored at the global level anyway, so I don't think it's a problem.

@rikkimax this change to .di generation has to happen regardless. I'm surprised nobody brought it up before that I recall.

@WalterBright WalterBright force-pushed the fix23658-2 branch 5 times, most recently from 953a6ab to c271384 Compare January 29, 2023 03:09
@WalterBright
Copy link
Member Author

this is ready to go

Copy link
Member

@rainers rainers left a comment

Choose a reason for hiding this comment

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

LGTM, though still a bit low on test cases.

BTW: not related to this PR, but the di file generated from header1.d doesn't compile:

header1.di(666): Error: undefined identifier `tuple`
header1.di(666): Error: undefined identifier `tuple`

@WalterBright
Copy link
Member Author

not related to this PR, but the di file generated from header1.d doesn't compile:

You know what to do: file a bugzilla issue!

@RazvanN7 RazvanN7 closed this Jan 31, 2023
@RazvanN7 RazvanN7 reopened this Jan 31, 2023
@RazvanN7 RazvanN7 merged commit b312925 into dlang:master Jan 31, 2023
@WalterBright WalterBright deleted the fix23658-2 branch January 31, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants