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 23597 - .di files not compatible with -i #14783

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

CyberShadow
Copy link
Member

Makes .di files more useful for e.g. Deimos projects.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @CyberShadow!

Bugzilla references

Auto-close Bugzilla Severity Description
23597 enhancement .di files not compatible with -i

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#14783"

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

this is such a trivial change, did you want to do this straight stable? Also rebase to fix CI errors.

@dkorpel
Copy link
Contributor

dkorpel commented Jan 4, 2023

If I understand correctly, this change makes -i not compile .di imports anymore. How does that solve a missing reference at link time? I would expect this has the opposite effect.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 4, 2023

-i promotes included modules to root modules, therefore the instance of fun is then generated in the object file of the .di file.

@dkorpel
Copy link
Contributor

dkorpel commented Jan 4, 2023

the instance of fun is then generated in the object file of the .di file.

So dmd doesn't pass object files from .di files to the linker?

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 4, 2023

I think that object files are never generated for .di files.

@CyberShadow CyberShadow force-pushed the pull-20230104-040820 branch from 9b3ba51 to 474f36c Compare January 4, 2023 14:56
@CyberShadow
Copy link
Member Author

Also rebase to fix CI errors.

Thanks, done.

this is such a trivial change, did you want to do this straight stable?

I am slightly worried about regressions for the case where a .d file is compiled separately, a .di file is generated from it, and then used to compile the main program, then linking the .d file's object file.

@CyberShadow
Copy link
Member Author

CyberShadow commented Jan 4, 2023

If I understand correctly, this change makes -i not compile .di imports anymore. How does that solve a missing reference at link time? I would expect this has the opposite effect.

So what this does is essentially tell the compiler that the corresponding module is not going to be compiled. .di files are already never compiled by themselves.

There are two cases for .di files:

  1. There is a .d file which contains the implementation, and a .di file which contains just the declarations. This is equivalent to C's classic .c + .h pairing.

    For this case, the compiler can assume that anything declared in the .di file will be compiled separately and available at link time.

  2. There is no .d file, there is just a .di file. This is equivalent to "header-only" C libraries, and is what we want to do with Deimos (because they attempt to be a 1:1 port of .h files in /usr/include).

    For this case, the compiler needs to be told that there will not be a .d file, and things like template instantiations will not be available at link time, so anything which is not explicitly implementation-less should be compiled and built as part of the current compilation.

What this change does is that it changes how the compiler treats imported .di files when -i is specified from 1 to 2.

In theory this might be a breaking change for the intentional 1 case, but considering that the -i feature is rarely used in practice (Dub doesn't use it, which is a shame because it can greatly speed up builds and reduce the filesize of built binaries), and combined with also the limited use of .di files, I think the real-world effect of this is going to be minimal.

@RazvanN7 RazvanN7 merged commit 5ef67b4 into dlang:master Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants