Refactor Objective-C frontend integration by jacob-carlborg · Pull Request #6679 · dlang/dmd

jacob-carlborg · 2017-04-06T20:04:00Z

The major reason for this refactoring is to move the decision if the Objective-C integration is enabled or not from a compile time (when building the compiler) decision using the makefile to a runtime (when running the compiler) decision. This allows to remove the extra objc_stubs.d file and is required for a cross-compilers like LDC, GDC and when DMD is cross-compiling for x86 64bit <-> 32bit.

I will followup with a pull request for refactoring the glue layer parts in the same way when this pull request is accepted.

ibuclaw · 2017-04-06T20:20:50Z

Also the C++ headers need updating with all necessary removals/additions, but otherwise a welcome step in the right direction.

WalterBright · 2017-04-06T20:24:43Z

src/ddmd/objc.d

I'm not a big fan of trailing _. We don't use it anyplace else.

What do you suggest instead, a leading _? The trailing underscore is there to avoid conflict with the objc function [1].

[1] https://github.com/dlang/dmd/pull/6679/files#diff-6c74f81a6d424346a88e4920f9f16869R245

I switch the trailing underscore to a leading.

WalterBright · 2017-04-06T20:25:59Z

src/ddmd/mars.d

_init isn't a great name, but perhaps we should be consistent here.

Ok, so to fix this name and remove the renamed import, should I move the initialize function to a static method inside the Objc interface and name it _init?

The initialize function has been moved into the Objc interface as a static method and renamed to _init. At the call site it's now: Objc._init();.

WalterBright · 2017-04-06T20:26:49Z

src/ddmd/mars.d

We don't use this style anyplace else in dmd.

I'll remove the renamed import.

It's a bit difficult to know which D features can be used. Are there some guidelines for DMD somewhere?

WalterBright · 2017-04-06T20:27:18Z

I like this much better than the last one. Some gripes about names used.

ibuclaw · 2017-04-06T20:30:53Z

I like this much better than the last one. Some gripes about names used.

Last one being #4813 ? :-)

jacob-carlborg · 2017-04-07T10:51:54Z

Also the C++ headers need updating with all necessary removals/additions, but otherwise a welcome step in the right direction.

Yes, I forgot about that.

A general question, what does need to be exposed as extern(C++), the whole AST?

jacob-carlborg · 2017-04-07T10:52:22Z

I like this much better than the last one. Some gripes about names used.

Thanks, I'll fix the names.

ibuclaw

A general question, what does need to be exposed as extern(C++), the whole AST?

Added notes inline.

ibuclaw · 2017-04-07T16:35:54Z

src/ddmd/objc.d

I guess remove everything except struct ObjcSelector from objc.h.

However in my C++ port, I'll probably include class Objc { }; with all these virtual functions. So I'm on the sideline here, as I don't want to deviate the C++ interface too much from upstream, but at the same time, this will be a non-issue once switching to D front-end.

jacob-carlborg · 2017-04-07T19:03:12Z

A general question, what does need to be exposed as extern(C++), the whole AST?

Added notes inline.

The question was a bit off-topic, but I was referring to DMD in general and not specific to this PR.

ibuclaw · 2017-04-07T19:09:23Z

I was referring to DMD in general and not specific to this PR.

In the strictest sense, anything that is used by the codegen routines. Just what is used depends on the compiler. It sometimes requires a shout out to prevent things being internalise as extern(D).

jacob-carlborg · 2017-04-07T19:11:37Z

In the strictest sense, anything that is used by the codegen routines. Just what is used depends on the compiler. It sometimes requires a shout out to prevent things being internalise as extern(D).

Ok, thanks. I assume GDC and LDC uses their own glue layer and not the same as DMD, so they would need more declarations as extern(C++) than DMD?

ibuclaw · 2017-04-07T19:23:11Z

Ok, thanks. I assume GDC and LDC uses their own glue layer and not the same as DMD, so they would need more declarations as extern(C++) than DMD?

Indeed, there may be information about the compilation that gdc/ldc uses that dmd does not. But it also works the other way, as there is a lot of parts that are dmd specific too - almost littered, infact. ☺️

jacob-carlborg · 2017-04-07T19:24:44Z

@ibuclaw I updated the header files. Could you please verify those changes?

ibuclaw · 2017-04-07T19:32:26Z

src/ddmd/objc.d

I see you put it in the headers, in which case this should be extern(C++) interface.

Yes, of course, I forgot that.

Fixed. I also made the constructors extern(D).

@ibuclaw The change to extern(C++) seems to cause every platform to fail in the auto tester. But it works fine locally (on macOS).

I'll try the patch locally (linux 64bit)

However looking at the travisci builds, it looks like it's only gdc that fails. And pretty much all autotester builds. So maybe we're triggering some lament C++ class bug in the frontend that has since fixed (I can't reproduce with gdc master). I think the autotester uses 2.067.

What happens if you exchange interface Objc with class Objc ?

Just pushed that change.

Seems to work better. At least the druntime build now passes.

Looks like that fixed it. Or at least it has built phobos w/o hitting a segv.

ibuclaw · 2017-04-07T19:34:44Z

src/ddmd/objc.d

-    ObjcSelector* selector;
-
-    extern (D) this(FuncDeclaration fdecl)
+    static void _init()


Oh! I apologise as I forgot, this static method should definitely be extern(C++) at the very least, because gdc's entrypoint is in C++.

ibuclaw · 2017-04-07T19:37:34Z

src/ddmd/objc.d

+    static void _init()
    {
-        this.fdecl = fdecl;
+        if (global.params.isOSX && global.params.is64bit)


This condition is OK for now, but I'd like to move it to somewhere else in future. Maybe some Compiler interface, as Target doesn't quite fit.

dnadlinger · 2017-04-07T22:23:55Z

Ok, thanks. I assume GDC and LDC uses their own glue layer and not the same as DMD, so they would need more declarations as extern(C++) than DMD?

Yes. Until the interface is split up or refactored into a more compartmentalized structure, LDC basically needs access to the full AST (not all fields and methods are used at any given time, of course, but I believe there are a few cases where we use stuff in our glue code that DMD doesn't.)

ibuclaw

Looks good to me.

ibuclaw · 2017-04-08T10:54:55Z

src/ddmd/objc.d

Private should be extern(D) too.

WalterBright · 2017-04-08T17:53:52Z

src/ddmd/aggregate.h

Another name nit. Note the subsequent isscope and isabstract as all lower case. isObjc looks like a function name, perhaps isobjc is better and more consistent with other usage.

Ok, I can fix that. I tried to go with the standard D naming conventions.

WalterBright · 2017-04-09T01:34:33Z

I don't see a button where I can approve this, but I do approve it.

PetarKirov · 2017-04-09T01:42:27Z

@WalterBright if you go to the " Files changed" tab, you should see a "Review changes button". When you click it, you should get three options: Request changes, Approve and Comment.

jacob-carlborg · 2017-04-09T08:00:32Z

I would like to squash the commits before this is merged.

Dismissed.

ibuclaw

Squash/merge changes.

jacob-carlborg · 2017-04-17T08:46:09Z

Rebased.

jacob-carlborg · 2017-04-17T09:03:03Z

The error is now the same as before:

../dmd/src/ddmd/func.d(39): Error: module objc is in file 'ddmd/objc.d' which cannot be read

This file is now shared for all platforms after the Objective-C integration refactoring: dlang/dmd#6679.

jacob-carlborg · 2017-04-17T09:23:52Z

@ibuclaw Found the issue why DAutoTest was failing. I created another PR for that: dlang/dlang.org#1636.

jacob-carlborg · 2017-04-18T13:42:09Z

@ibuclaw @WalterBright All tests are green now except for DAutoTest which needs this PR dlang/dlang.org#1636 and vibe.d which looks unrelated (other PRs have the same issue).

jacob-carlborg · 2017-04-21T15:30:47Z

@ibuclaw @WalterBright is there anything stopping this from being merged?

ibuclaw · 2017-04-21T17:30:05Z

src/ddmd/dclass.d

Nit-pick, can we remove the scope parameter here? It's only used for checking sc.linkage, which can be instead moved here.

I'll fix this.

ibuclaw · 2017-04-21T17:33:22Z

src/ddmd/objc.h


-struct Objc_ClassDeclaration
+class Objc
 {


Insert public: here.

I'll fix this.

ibuclaw · 2017-04-21T17:43:03Z

@ibuclaw @WalterBright is there anything stopping this from being merged?

Need public: in the objc header which allows me to call Objc::_init from GDC, and to backport this patch for the benefit of the C++ frontend. :-)

ibuclaw · 2017-04-22T07:12:41Z

Built on gdc with my two requested changes applied. Looks good, so I'm just awaiting your fix ups.

jacob-carlborg · 2017-04-22T10:38:11Z

@ibuclaw everything fixed and rebased.

ibuclaw · 2017-04-22T11:22:28Z

src/ddmd/objc.h

Aren't we forgetting something? 😉

I truly hate header files 😃. Fixed.

The major reason for this refactoring is to move the decision if the Objective-C integration is enabled or not from a compile time (when building the compiler) decision using the makefile to a runtime (when running the compiler) decision. This allows to remove the extra objc_stubs.d file and is required for a cross-compilers like LDC, GDC and when DMD is cross-compiling for x86 64bit <-> 32bit.

jacob-carlborg · 2017-04-22T13:21:16Z

@ibuclaw All tests are green now.

ibuclaw · 2017-04-22T13:31:24Z

Ping @WalterBright - I assume you are an admin who can merge this? dlang/dlang.org#1636 should fix the failing DAutoTest check, but requires this be in first.

jacob-carlborg · 2017-04-25T06:55:23Z

@WalterBright ping, Iain already toggled this for auto merging, but this other PR is necessary as well for dlang.org: dlang/dlang.org#1636.

WalterBright · 2017-04-25T08:31:54Z

Auto-merge toggled on

jacob-carlborg · 2017-04-25T11:18:48Z

Thanks.

This file is now shared for all platforms after the Objective-C integration refactoring: dlang/dmd#6679.

MartinNowak · 2017-04-25T11:38:17Z

Doc build fixed by dlang/dlang.org#1636.

WalterBright reviewed Apr 6, 2017

View reviewed changes

ibuclaw reviewed Apr 7, 2017

View reviewed changes

jacob-carlborg force-pushed the objc_refactor branch from e1f67db to e6ac8aa Compare April 7, 2017 19:19

ibuclaw requested changes Apr 7, 2017

View reviewed changes

jacob-carlborg force-pushed the objc_refactor branch 2 times, most recently from 825296a to 0003536 Compare April 8, 2017 10:05

ibuclaw requested changes Apr 8, 2017

View reviewed changes

jacob-carlborg force-pushed the objc_refactor branch from 0003536 to 327a5f7 Compare April 8, 2017 15:09

WalterBright reviewed Apr 8, 2017

View reviewed changes

jacob-carlborg force-pushed the objc_refactor branch from 862671b to 844266f Compare April 8, 2017 18:00

WalterBright approved these changes Apr 9, 2017

View reviewed changes

ibuclaw previously approved these changes Apr 9, 2017

View reviewed changes

ibuclaw requested changes Apr 9, 2017

View reviewed changes

jacob-carlborg force-pushed the objc_refactor branch from a33e12e to 69a50ac Compare April 17, 2017 08:45

jacob-carlborg added a commit to jacob-carlborg/dlang.org that referenced this pull request Apr 17, 2017

Remove special casing for objc.d from the Posix makefile

1c54eed

This file is now shared for all platforms after the Objective-C integration refactoring: dlang/dmd#6679.

jacob-carlborg mentioned this pull request Apr 17, 2017

Remove special casing for objc.d from the Posix makefile dlang/dlang.org#1636

Merged

jacob-carlborg force-pushed the objc_refactor branch from 69a50ac to 7fef02e Compare April 18, 2017 11:51

ibuclaw reviewed Apr 21, 2017

View reviewed changes

jacob-carlborg force-pushed the objc_refactor branch from 7fef02e to 1f5f2e9 Compare April 22, 2017 10:37

jacob-carlborg force-pushed the objc_refactor branch from 1f5f2e9 to 62cb676 Compare April 22, 2017 11:18

ibuclaw requested changes Apr 22, 2017

View reviewed changes

jacob-carlborg force-pushed the objc_refactor branch from 62cb676 to 9fe35b1 Compare April 22, 2017 11:23

ibuclaw approved these changes Apr 22, 2017

View reviewed changes

ibuclaw added the Merge:auto-merge label Apr 22, 2017

WalterBright merged commit c142eaa into dlang:master Apr 25, 2017

jacob-carlborg deleted the objc_refactor branch April 25, 2017 11:18

schuetzm pushed a commit to dlang/dlang.org that referenced this pull request Apr 25, 2017

Remove special casing for objc.d from the Posix makefile (#1636)

d581bd3

This file is now shared for all platforms after the Objective-C integration refactoring: dlang/dmd#6679.

Uh oh!

Conversation

jacob-carlborg commented Apr 6, 2017

Uh oh!

ibuclaw commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Apr 6, 2017

Uh oh!

ibuclaw commented Apr 6, 2017

Uh oh!

jacob-carlborg commented Apr 7, 2017

Uh oh!

jacob-carlborg commented Apr 7, 2017

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented Apr 7, 2017

Uh oh!

ibuclaw commented Apr 7, 2017

Uh oh!

jacob-carlborg commented Apr 7, 2017

Uh oh!

ibuclaw commented Apr 7, 2017

Uh oh!

jacob-carlborg commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger commented Apr 7, 2017

Uh oh!

ibuclaw left a comment

Choose a reason for hiding this comment

ibuclaw commented Apr 6, 2017 •

edited

Loading

jacob-carlborg Apr 7, 2017 •

edited

Loading