Refactoring of the Objective-C integration by jacob-carlborg · Pull Request #5766 · dlang/dmd

jacob-carlborg · 2016-05-11T20:13:47Z

I'm not sure if there's a better place for the functions in objc_glue.c.

jacob-carlborg · 2016-05-11T20:14:08Z

@yebblies Refactoring of the Objective-C integration

WalterBright · 2016-05-11T23:15:29Z

Can you give a brief description of the purpose of the refactoring? My cursory look at it is it appears to move ObjC logic out of objc modules and dispersing it into the rest of the front end. Why?

jacob-carlborg · 2016-05-12T06:40:03Z

Currently the Objective-C integration is the only feature that is organized in this, slightly weird, way. With this refactoring it brings the feature up to the same level as the rest of the features
It doesn't work with cross-compiling which is important for LDC and might become important later for DMD as well. Currently all the Objective-C logic is removed at compile time if the target doesn't support Objective-C. The decision if Objective-C is supported or not need to occur at runtime and not compile time for cross-compiling to work
This all the functions with the weird names that was a result of being force to moved out logic that should have been inline in the rest of the code
It simplifies the design because objc.d, objc_stubs.d and objc_glue_stubs.c are all removed

I talked with Daniel and Iain during dconf about this and they both think that this is the right decision. I'm pretty sure that David agrees as well.

jacob-carlborg · 2016-05-12T06:52:19Z

Hmm, seems I need to give this refactoring some more thought.

WalterBright · 2016-05-12T16:16:47Z

the only feature that is organized in this, slightly weird, way.

No, it's not. Header generation, json generation, ddoc, array ops, etc., are all factored out into separate modules. I would like to increase this sort of encapsulation, not decrease it. The main modules are already far too large and convoluted. Distributing the objc support among them will make things more convoluted and harder to understand.

Your other goals with this are good, but I do not believe they require distributing their contents around the rest of the code to achieve them.

yebblies · 2016-05-12T16:22:38Z

Those other features are not adding new types, semantic processing, etc. We do not have a nice system for factoring out extern language support in dmd, and rather than making it clearer this layout just makes a mess. An entire function for two lines of code? It's a joke, and it doesn't work.

Either this is a first-class feature that deserves being integrated with the rest of dmd, or we should delete it.

ibuclaw · 2016-05-12T18:30:50Z

Objective-C implementation is pretty dumb, I couldn't find a way to integrate it into GDC without #4813

ibuclaw · 2016-05-12T18:33:25Z

Everything should have been put into an Objc namespace from the start. extern C functions are not a form of encapsulation.

dnadlinger · 2016-05-12T21:10:42Z

Either this is a first-class feature that deserves being integrated with the rest of dmd, or we should delete it.

This.

WalterBright · 2016-05-12T21:31:26Z

I am interested in proper encapsulation in order to reduce bugs and complexity. I am not interested in status symbols of where code is located. If objc is support is lacking (and I don't know if it is or is not) we can address that.

An entire function for two lines of code? It's a joke, and it doesn't work.

We have lots of functions in dmd that are a couple lines. They serve encapsulation purposes. As I recall you asked for a one-liner in root/outbuffer yesterday (a length() function). Besides, func.d is acquiring a number of functions with many more than 2 lines of code in them.

The front end, in my not-so-humble opinion, has major problems with lack of encapsulation. The lack of encapsulation is not a justification for even less encapsulation.

dnadlinger · 2016-05-12T21:46:44Z

The frontend definitely suffers from a lot of encapsulation problems. The question is just whether "Objective-C support" is an useful unit of encapsulation.

jacob-carlborg · 2016-05-13T06:18:57Z

If objc is support is lacking (and I don't know if it is or is not) we can address that

That's what I intend to work on. After the huge 12k+ lines of code PR which was the initial one, we decided to split it up in more manageable pieces. The rest of the Objective-C support has not been integrated yet. I talked with Daniel at dconf and we agreed that we should do this refactoring first (assuming this is what he had in mind).

jacob-carlborg · 2016-05-13T06:19:59Z

"Objective-C support" is an useful unit of encapsulation

The full integration will touch many (most) parts of the compiler.

ibuclaw · 2016-05-13T06:35:16Z

src/aggregate.h

    TypeInfoClassDeclaration *vclassinfo;       // the ClassInfo object for this ClassDeclaration
    bool com;                           // true if this is a COM class (meaning it derives from IUnknown)
    bool cpp;                           // true if this is a C++ interface
+    bool objc;                          // true if this is an Objective-C class/interface


Classes can only be com, cpp, or objc, but never two at once, right? These bool flags could be squashed into an enum later, perhaps.

Yes, as far as I know. If it's objc it cannot be anything else at least. I guess a new PR would be a better fit for that.

IIRC there is at least one other way to mark a class as c++, it could do with some cleaning up.

yebblies · 2016-05-13T07:08:39Z

I am interested in proper encapsulation in order to reduce bugs and complexity. I am not interested in status symbols of where code is located. If objc is support is lacking (and I don't know if it is or is not) we can address that.

My point is that unless we're planning to delete it (and therefore want it nicely isolated and easy to delete) then we should put the parts where they logically make sense - next to the handling of other extern types in dmd.

We have lots of functions in dmd that are a couple lines. They serve encapsulation purposes.

These functions do not serve encapsulation purposes. They are simply manually outlined chunks of code, that only have one caller. If your concern is that it might not be immediately obvious what they're for, then adding comments would be a less disruptive solution.

As I recall you asked for a one-liner in root/outbuffer yesterday (a length() function).

Because it give a name to a common pattern... there is a difference here.

The front end, in my not-so-humble opinion, has major problems with lack of encapsulation. The lack of encapsulation is not a justification for even less encapsulation.

I would agree with you if I thought that outlining objective-C support into another file improved encapsulation. Instead it just hides the code from people editing the related functions, and adds more false dependencies between modules.

Just look at how the line count changes...

jacob-carlborg · 2016-05-13T07:15:35Z

BTW, I added documentation for most of the remaining functions.

Any reason why the Windows builds are still pending? It looks like it already has built twice on the other platforms.

jacob-carlborg · 2016-05-13T08:04:23Z

I assume the Windows failures are not related: core.exception.AssertError@std\datetime.d(28135): TZName which is missing: Easter Island Standard Time

Hackerpilot · 2016-05-13T08:13:00Z

@jacob-carlborg: dlang/phobos#4317

jacob-carlborg · 2016-05-13T09:19:20Z

@Hackerpilot thanks. Can we rerun the Windows tests?

jacob-carlborg · 2016-05-13T20:38:12Z

Awesome, all tests are passing now.

WalterBright · 2016-05-14T00:24:26Z

src/declaration.h

    void accept(Visitor *v) { v->visit(this); }
 };

+struct ObjcSelector


This should go in objc.d, there's no reason to make the fields and methods available throughout the compiler.

WalterBright · 2016-05-14T00:26:06Z

src/e2ir.c

        Symbol *sfunc = toSymbol(fd);

-        if (esel)
+        if (global.params.hasObjectiveC && esel)


I don't see how this is an improvement.

WalterBright · 2016-05-14T00:58:43Z

I object to moving the code into func.d. I don't see how that improves anything. It reduces encapsulation by making the objc structs and members visible everywhere. I don't see how it makes anything easier to understand.

jacob-carlborg · 2016-05-14T13:25:43Z

Could we please come to some form of agreement here. I would like to continue with the Objective-C integration but we need to decide how to structure the code.

WalterBright · 2016-05-14T21:18:28Z

It simplifies the design because objc.d, objc_stubs.d and objc_glue_stubs.c are all removed

Removing files has no correlation with simplifying the design. But I see no reason why the ObjC logic cannot all be contained in objc.d, objc.h, and objc_glue.c. Then, one only has 3 files to look at to follow the ObjC design and figure out how it works.

ibuclaw · 2016-05-15T07:00:00Z

Could we please come to some form of agreement here?

I would say that all data structures and functions prefixed with objc_ClassDeclaration and objc_FuncDeclaration should be first class citizens of their respective classes. Having an objc param in globals is not strictly necessary as we can put it elsewhere, such as in Objc::isSupported.

Other routines that hide the internals implementation should be in an Objc interface. Think how Target or Port works.

yebblies · 2016-05-15T07:32:22Z

Having an objc param in globals is not strictly necessary as we can put it elsewhere, such as in Objc::isSupported.

I'd rather have global state in globals where possible.

jacob-carlborg · 2016-05-31T13:08:35Z

Yet another PR that is stalled. @WalterBright you seem to be the only one against this change.

WalterBright · 2016-06-01T08:34:55Z

Yes, I am against it, for reasons stated.

jacob-carlborg · 2016-06-01T18:18:14Z

You want encapsulation, but way the current Objective-C integration is written doesn't increase encapsulation.

You mentioned that header generation, json generation and ddoc all have separate files. But the (I assume) do not need to modify the AST in anyway, they just read the AST and generate the output.

jacob-carlborg · 2016-06-17T06:49:35Z

@WalterBright @yebblies what if we do a compromise. If I inline calls like objc_ClassDeclaration_semantic_PASSinit_LINKobjc and objc_callfunc_setupMethodSelector but keep code like struct ObjcSelector and validateSelector in the objc.d/c file?

WalterBright · 2016-06-20T10:53:27Z

I don't understand what the proposed compromise does.

jacob-carlborg · 2016-06-20T19:50:08Z

I don't understand what the proposed compromise does.

@WalterBright I have not changed any code yet in the PR. Currently the definition of objc_ClassDeclaration_semantic_PASSinit_LINKobjc looks like this [1] and is used here [2]. My proposed compromise is to move that function inline at the call site to look like this (in dclass.d):

if (sc.linkage == LINKobjc)
{
    if (global.params.hasObjectiveC)
        objc = true;
    else
        error("Objective-C classes are not supported for this target");
}

Other symbols like ObjcSelector would be moved back to objc.d. If you think it's ok that the objc_ClassDeclaration_semantic_PASSinit_LINKobjc function is inline at the call site I can update the PR to show how it would look like with the rest of the symbols.

[1] https://github.com/dlang/dmd/blob/master/src/objc.d#L143
[2] https://github.com/dlang/dmd/blob/master/src/dclass.d#L500-L501

WalterBright · 2016-06-20T22:30:21Z

Thanks for the explanation. I honestly don't see the point of making this change. How does it improve things?

jacob-carlborg · 2016-06-21T08:18:25Z

Thanks for the explanation. I honestly don't see the point of making this change. How does it improve things?

@WalterBright it would remove the awkward functions like objc_ClassDeclaration_semantic_PASSinit_LINKobjc. They're awkward because they should really not be separate functions. They're only called once and doesn't provide any form of abstraction.

wilzbach · 2016-08-07T13:56:52Z

JC: Could we please come to some form of agreement here. I would like to continue with the Objective-C integration but we need to decide how to structure the code.
[...]
JC: Yet another PR that is stalled. @WalterBright you seem to be the only one against this change.
WB: Yes, I am against it, for reasons stated.

@jacob-carlborg did you ever come to an agreement or is this still stalled?

ibuclaw · 2016-08-07T17:15:44Z

This is still stalled as far as I know. Which is a shame I think it's in the interest of everyone to fix this interface.

jacob-carlborg · 2016-08-07T19:56:36Z

@wilzbach it's still stalled. Therefore the rest of Objective-C integration is stalled.

jacob-carlborg force-pushed the objc_refactor branch from 0fa2a8f to 1845ead Compare May 12, 2016 06:48

jacob-carlborg force-pushed the objc_refactor branch 4 times, most recently from 9bb6cc8 to c495956 Compare May 12, 2016 08:37

Refactor the Objective-C integration

c1a6558

jacob-carlborg force-pushed the objc_refactor branch from c495956 to c820a37 Compare May 12, 2016 11:45

ibuclaw reviewed May 13, 2016
View reviewed changes

WalterBright reviewed May 14, 2016
View reviewed changes

Add documentation for the Objective-C integration

ee5fe1f

jacob-carlborg force-pushed the objc_refactor branch from c820a37 to ee5fe1f Compare May 14, 2016 13:23

jacob-carlborg closed this Apr 2, 2017

jacob-carlborg deleted the objc_refactor branch April 2, 2017 11:03

Uh oh!

Comments

Conversation

jacob-carlborg commented May 11, 2016

Uh oh!

jacob-carlborg commented May 11, 2016

Uh oh!

WalterBright commented May 11, 2016

Uh oh!

jacob-carlborg commented May 12, 2016

Uh oh!

jacob-carlborg commented May 12, 2016

Uh oh!

WalterBright commented May 12, 2016

Uh oh!

yebblies commented May 12, 2016

Uh oh!

ibuclaw commented May 12, 2016

Uh oh!

ibuclaw commented May 12, 2016

Uh oh!

dnadlinger commented May 12, 2016

Uh oh!

WalterBright commented May 12, 2016

Uh oh!

dnadlinger commented May 12, 2016

Uh oh!

jacob-carlborg commented May 13, 2016

Uh oh!

jacob-carlborg commented May 13, 2016

Uh oh!

ibuclaw May 13, 2016

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg May 13, 2016

Choose a reason for hiding this comment

Uh oh!

yebblies May 13, 2016

Choose a reason for hiding this comment

Uh oh!

yebblies commented May 13, 2016

Uh oh!

jacob-carlborg commented May 13, 2016

Uh oh!

jacob-carlborg commented May 13, 2016

Uh oh!

Hackerpilot commented May 13, 2016

Uh oh!

jacob-carlborg commented May 13, 2016

Uh oh!

jacob-carlborg commented May 13, 2016

Uh oh!

WalterBright May 14, 2016

Choose a reason for hiding this comment

Uh oh!

WalterBright May 14, 2016

Choose a reason for hiding this comment

Uh oh!

WalterBright commented May 14, 2016

Uh oh!

jacob-carlborg commented May 14, 2016

Uh oh!

WalterBright commented May 14, 2016

Uh oh!

ibuclaw commented May 15, 2016

Uh oh!

yebblies commented May 15, 2016

Uh oh!

jacob-carlborg commented May 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright commented Jun 1, 2016

Uh oh!

jacob-carlborg commented Jun 1, 2016

Uh oh!

jacob-carlborg commented Jun 17, 2016

Uh oh!

WalterBright commented Jun 20, 2016

Uh oh!

jacob-carlborg commented Jun 20, 2016

jacob-carlborg commented May 31, 2016 •

edited

Loading