Add C++ header generation by MoonlightSentinel · Pull Request #10801 · dlang/dmd

MoonlightSentinel · 2020-02-18T12:08:52Z

Squashed rebase of #9971 without the changes to the existing C++ tests (as proposed in #9971 (comment)).

dlang-bot · 2020-02-18T12:08:55Z

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + dmd#10801"

thewilsonator

Thanks! First review pass, Indentation:

thewilsonator · 2020-02-18T12:42:14Z

+                buf.printf(" {}\n");
+            }
+
+            version (none)


any ideas what this version none ins for?

Not sure but the versioned code generates an additional constructor to set all fields, e.g.

S2(_d_int a, _d_int b, _d_long c) { this->a = a; this->b = b; this->c = c; }

for

extern (C++) struct S2 { int a = 42; int b; long c; this(int a) {} }

Geod24

We could use a changelog entry describing the user-facing interface.
Things that immediately come to mind:

What happens when doing dmd a.d b.d c.d -HC ?
Mention the scope (only applies to extern(C) and extern(C++) declarations, ignore everything else)
Mention the limitations (semantic not run)

Geod24 · 2020-02-18T12:51:04Z

+    OutBuffer buf;
+    buf.writestring("#pragma once\n");
+    buf.writeByte('\n');
+    buf.printf("// Automatically generated by dmd -HC\n");


I'm wondering whether we should include the DMD version.
On one hand it's informative, on the other hand it would need to needless diff in the event a generated header is checked in and its generation is part of the process (it doesn't make sense to me, but I'm sure someone will do it).
Same issue would arise with a timestamp. However I can see a value in knowing a bit more about how the header was generated.

Last but not least, having the full command line might be helpful too. Things like -version and -debug will affect the output, won't they ?

Indeed, that could be useful to track down errors but also annoying for diffs (and maybe reproducible builds). Maybe this behavior should be customizable via an optional argument to -HC. But i think this could be added later.

Last but not least, having the full command line might be helpful too. Things like -version and -debug will affect the output, won't they ?

I think so.

At the very least, use __VENDOR__, as it's not only dmd that would use this option.

Geod24 · 2020-02-18T13:04:49Z

As a side note, something that would be a life-saver for people using C++ interop easily is an automatic offsetof / sizeof check (in the form of static assert). But currently semantic isn't run so that's not doable.

MoonlightSentinel · 2020-02-18T15:38:26Z

As a side note, something that would be a life-saver for people using C++ interop easily is an automatic offsetof / sizeof check (in the form of static assert). But currently semantic isn't run so that's not doable.

This could be added later. But for now the main goal is to integrate an initial version s.t. @TurkeyMan and other people can start contributing as well.

MoonlightSentinel · 2020-02-18T23:47:23Z

I've added a preliminary changelog but I'm unsure whether we should promote it right away.

Some remaining TODO's:

Actually implement generating separate header files (the directory passed to -HDd is currently treated as a prefix for the file passed to -HDf
Cleanup generated header files (e.g. decide whether #define or typedef is used)
(Opt-in) metadata as proposed in Add C++ header generation #10801 (comment)
Missing semantic analysis

But these should mostly be resolved in further PR's.

wilzbach · 2020-02-19T00:23:00Z

but I'm unsure whether we should promote it right away.

Maybe state "experimental" support? So that it's clear that not everything is fully tested and bug reports are welcome.

But these should mostly be resolved in further PR's.

Yes, please keep this one as small/low-effort as possible so that people can finally start testing it. Thank you very much for taking this to the finish line!!

MoonlightSentinel · 2020-02-19T02:02:29Z

Maybe state "experimental" support? So that it's clear that not everything is fully tested and bug reports are welcome.

Done

Yes, please keep this one as small/low-effort as possible so that people can finally start testing it. Thank you very much for taking this to the finish line!!

Gotcha. Maybe such big features should be developed on a dedicated branch in the future. That would offer similar benefits to a draft PR while also keeping the work publicly accessible (everbody can make a small PR without adopting the entire stalled PR)

RazvanN7 · 2020-02-19T02:35:03Z

@Geod24 @MoonlightSentinel Semantic is run before generating the cpp header files. The generateCPP function is called in mars.d after full semantic analysis has been performed. I think that the reason for this is that you may have structs that have fields that are template instances (T!int a). Also, you want to make sure that the code is correct before generating C++ header files.

wilzbach · 2020-02-19T02:48:16Z

Maybe such big features should be developed on a dedicated branch in the future. That would offer similar benefits to a draft PR while also keeping the work publicly accessible (everbody can make a small PR without adopting the entire stalled PR)

FYI: We have tried this before and even made nightly builds of these branches available. No one ever downloaded those (I guess it also was a problem of people not knowing about it). Additionally, you run the high risk of the branch having lots of merge conflicts with master, so IMHO the best thing we can do are feature flags (in some way, e.g. -preview or now this flag).

RazvanN7 · 2020-02-19T02:50:26Z

Actually implement generating separate header files (the directory passed to -HDd is currently treated as a prefix for the file passed to -HDf

This is not possible (should be read as hard to implement) for the dmd codebase, due to the differences between C++ and D when it comes to importing (vs includes) and forward references.

@edi33416 tried this but it didn't work for the dmd codebase (I can't remember the reason though, it was something about having duplicate forward declarations). You can still generate individual header files, it's just that you have to invoke the compiler for every .d file separately.

MoonlightSentinel · 2020-02-19T19:03:05Z

@Geod24 @MoonlightSentinel Semantic is run before generating the cpp header files. The generateCPP function is called in mars.d after full semantic analysis has been performed. I think that the reason for this is that you may have structs that have fields that are template instances (T!int a). Also, you want to make sure that the code is correct before generating C++ header files.

That is (sadly) not entirely true as some checks are delayed until IR / code generation (and hence occur after the header generation). E.g.

extern(C++) void foo(string s) {}

fails to compile

Error: Internal Compiler Error: type string cannot be mapped to C++

but does not issue an error when using -c -o-. Then -HC still generates a header containing

extern _d_void foo(DArray<_d_char> s);

MoonlightSentinel · 2020-02-19T19:09:01Z

FYI: We have tried this before and even made nightly builds of these branches available.

Thanks, I did not know that. Seems like it's usually not the best idea.

MoonlightSentinel · 2020-02-19T19:13:12Z

This is not possible (should be read as hard to implement) for the dmd codebase, due to the differences between C++ and D when it comes to importing (vs includes) and forward references.

Agreed, this will be a bigger project for the future but i think it could be workable (at least by automating the proposed workaround)

MoonlightSentinel · 2020-02-19T19:15:52Z

What's up with the weird spacing and random braces in the generated changelog?

MoonlightSentinel · 2020-02-19T22:50:12Z

Eliminated the remaining style issues and got the tests passing by switching from separate files to TEST_OUTPUT sections (hence it currently includes #10807 to avoid duplication for -m32 and -m32mscoff).

wilzbach · 2020-02-19T23:17:35Z

What's up with the weird spacing and random braces in the generated changelog?

Fix: dlang/tools#390

- Documentation for dtoh.d - Removed redundant static - Add missing members to globals.h - Generate .h instead of .out files in tests - Add a changelog

thewilsonator

Otherwise looks good to me.

RazvanN7 · 2020-02-21T08:04:37Z

So what's the plan to remove all the .h files from dmd-fe? We won't be able to market this feature if we aren't using it in-house.

thewilsonator · 2020-02-21T08:09:48Z

Step 0: merge this.
Step 1: let @TurkeyMan submit bug reports
Step 2: Fix them.
Step 3: Wait for @ibuclaw to get this into a GCC release.
Step 4: Wait again for that release to be the version used to build subsequent versions of GCC.
Step 5: Make the release that contains fixes from step 2 the oldest version used to build DMD.
Step 6: rm -rf *.h

RazvanN7 · 2020-02-21T08:12:28Z

Let's get this in! It seems that the buildkite failure is due to some version mismatch. probably unrelated to this PR.

Geod24 · 2020-02-21T08:14:03Z

Let's get this in! It seems that the buildkite failure is due to some version mismatch. probably unrelated to this PR.

Correct: dlang/ci#410

Geod24 · 2020-02-21T08:15:13Z

I'll try to give this a review this weekend.

wilzbach · 2020-02-21T08:25:13Z

Step 3: Wait for @ibuclaw to get this into a GCC release.
Step 4: Wait again for that release to be the version used to build subsequent versions of GCC.
Step 5: Make the release that contains fixes from step 2 the oldest version used to build DMD.
Step 6: rm -rf *.h

All these steps aren't required. We talked about this before. It's no problem because:
A) we can check-in the header file into source code (with a CI check enforcing this)
B) DMD frontend requires a working D compiler, so it's absolutely reasonable to except that the build.d build script will be able to generate the headers
I think we still want to go with option A for now as it will provide LDC/GDC with a nice git history of all header file changes for a release.

- remove unused variables - sort import in mars.d - remove unecessary braces - pointer style - replace duplication with goto - TODO comments for missing implementation - Replace Type[...].ty check + cast with isType[...]

- Remove leftover comment - Guard against multiple initializations - Add a dshell test

wilzbach

Awesome! Glad to get this finally started!

TurkeyMan · 2020-02-21T18:13:36Z

🎉🎉🎉

Awesome! Thank you so much everyone for working this through!

ibuclaw · 2020-02-22T21:58:27Z

A) we can check-in the header file into source code (with a CI check enforcing this)

I think this was my suggestion, it reminds contributors to regenerate the headers (it can be a make rule) before submitting the PR. Keeping it checked in is harmless anyway.

There will always need to be a header present for the initial bootstrap (assume that for the next 2-3 years, the baseline D compiler doesn't have -HC), otherwise you won't be able to build the C++ parts of gdc and ldc..

ibuclaw · 2020-02-22T22:01:54Z

Also, this needs a little extra work, as the module in its current form both imports dmd-specific modules and doesn't export the main entrypoint to C++.

Geod24 · 2020-02-24T12:30:53Z

Templates seems completely unhandled, and due to the way the FE treats alias, I doubt they will be simple to handle.
Also while I was playing around with this I accidentally hit an ICE: https://issues.dlang.org/show_bug.cgi?id=20604

yebblies · 2020-03-05T14:01:47Z

From 2015-09-16 to 2020-02-22! (#5082 #8591 #9971 #10801) It lives!

ibuclaw · 2020-03-10T17:20:31Z

@yebblies glad to hear from you as always. :-)

MoonlightSentinel requested review from Geod24, MartinNowak, WalterBright, ibuclaw and rainers as code owners February 18, 2020 12:08

thewilsonator reviewed Feb 18, 2020

View reviewed changes

Geod24 reviewed Feb 18, 2020

View reviewed changes

MoonlightSentinel force-pushed the headers branch from 00c74cd to 3585ac1 Compare February 18, 2020 18:53

MoonlightSentinel force-pushed the headers branch from 3585ac1 to 960e654 Compare February 19, 2020 01:45

MoonlightSentinel force-pushed the headers branch 2 times, most recently from f9ddcdb to 27fe7b0 Compare February 19, 2020 21:01

MoonlightSentinel mentioned this pull request Feb 19, 2020

Match "32mscoff" to "32" in conditional TEST_OUTPUT #10807

Merged

MoonlightSentinel force-pushed the headers branch from 27fe7b0 to 1b7fbc8 Compare February 19, 2020 22:20

wilzbach mentioned this pull request Feb 19, 2020

Fix changelog generation for markdown code blocks dlang/tools#390

Merged

MoonlightSentinel force-pushed the headers branch 2 times, most recently from 0ac8330 to ef47cce Compare February 21, 2020 00:24

MoonlightSentinel added 4 commits February 21, 2020 01:27

Address style feedback by thewilsonator

5132db2

Address Geod24's feedback

288d5c2

- Documentation for dtoh.d - Removed redundant static - Add missing members to globals.h - Generate .h instead of .out files in tests - Add a changelog

Replace version (BUILD_COMPILER) with a runtime check

b66e265

Move extra header files into TEST_OUTPUT

4b4172b

MoonlightSentinel force-pushed the headers branch from ef47cce to 9525e6f Compare February 21, 2020 00:30

thewilsonator approved these changes Feb 21, 2020

View reviewed changes

Comment thread src/dmd/dtoh.d Outdated

Comment thread src/dmd/dtoh.d Outdated

Comment thread src/dmd/dtoh.d Outdated

RazvanN7 approved these changes Feb 21, 2020

View reviewed changes

MoonlightSentinel added 2 commits February 21, 2020 11:44

Apply further cleanup

88fb68d

- remove unused variables - sort import in mars.d - remove unecessary braces - pointer style - replace duplication with goto - TODO comments for missing implementation - Replace Type[...].ty check + cast with isType[...]

Address wilzbach's review

d0d5f08

- Remove leftover comment - Guard against multiple initializations - Add a dshell test

MoonlightSentinel force-pushed the headers branch from 9525e6f to d0d5f08 Compare February 21, 2020 10:46

wilzbach approved these changes Feb 21, 2020

View reviewed changes

wilzbach merged commit 44112eb into dlang:master Feb 21, 2020

ibuclaw mentioned this pull request Feb 22, 2020

dmd.dtoh: export genCppHdrFiles to C++ #10821

Merged

moon-chilled mentioned this pull request Feb 23, 2020

Revive Generate C++ header files from public extern (C++) declarations #9971

Closed

MoonlightSentinel deleted the headers branch February 25, 2020 01:37

ibuclaw added the Feature:dtoh C++ header generation label Sep 28, 2020

Uh oh!

Conversation

MoonlightSentinel commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 18, 2020

Bugzilla references

Testing this PR locally

Uh oh!

thewilsonator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thewilsonator Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

MoonlightSentinel Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

Geod24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Geod24 Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

MoonlightSentinel Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

ibuclaw Feb 22, 2020

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Feb 18, 2020

Uh oh!

MoonlightSentinel commented Feb 18, 2020

Uh oh!

MoonlightSentinel commented Feb 18, 2020

Uh oh!

wilzbach commented Feb 19, 2020

Uh oh!

MoonlightSentinel commented Feb 19, 2020

Uh oh!

RazvanN7 commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Feb 19, 2020

Uh oh!

RazvanN7 commented Feb 19, 2020

Uh oh!

MoonlightSentinel commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoonlightSentinel commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoonlightSentinel commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoonlightSentinel commented Feb 19, 2020

Uh oh!

MoonlightSentinel commented Feb 19, 2020

MoonlightSentinel commented Feb 18, 2020 •

edited

Loading

RazvanN7 commented Feb 19, 2020 •

edited

Loading

MoonlightSentinel commented Feb 19, 2020 •

edited

Loading

MoonlightSentinel commented Feb 19, 2020 •

edited

Loading

MoonlightSentinel commented Feb 19, 2020 •

edited

Loading