DIP 43: D/Objective-C

[jacob-carlborg]

Here it finally is: support for extern(Objective-C). The matching pull request for druntime is here: dlang/druntime#957

http://wiki.dlang.org/DIP43

[WalterBright]

Thanks for doing this herculean task. I think it's very important.

But there are problems:

1. it's simply impossible to review, as it is too large and intrusive
2. github diff just hangs on it, I can't even view the changes in a reasonable way
3. it requires intimate knowledge of O-C, which I don't have
4. it will likely break with other changes to dmd
5. it is intimately interleaved with other dmd code

On the plus side, you have set it apart with # ifdef's. I suggest going further in that direction, to wit:

1. Abstract away all the semantic code that is currently set off with # ifdefs. Replace each with a function call, prefixed with objc_.
2. Place the implementations of all the objc_ functions in separate source files.
3. The objc_ functions all compile to nothing on platforms without O-C.
4. Separate out additions to dmd data structures as being structs, named with a Objc_ prefix, where the whole struct becomes a field, rather than as individual fields. This enables the struct to be abstracted away, as well as its construction, rather than having its bookkeeping implementation scattered through the dmd code.

By eliminating the # ifdefs, the code should become much more tractable, encapsulated, greppable. and convertible to DDMD. It should also become more resistant to requiring rebasing, and other PR's requiring rebasing.

I've made similar efforts to remove back end and platform dependencies from the front end, with good success.

[jacob-carlborg]

1. it's simply impossible to review, as it is too large and intrusive
2. github diff just hangs on it, I can't even view the changes in a reasonable way

I don't know what I can do about that besides splitting it up in multiple pull requests which might not be so easy. There's also the option to pull it down locally and look at the code.

3. it requires intimate knowledge of O-C, which I don't have

Do you see this as a major problem or is it more of a note? One person can obviously not know everything and it would be sad if this was a blocking issue.

5. it is intimately interleaved with other dmd code

Yeah, I don't see an easy way around that. The existing code is quite interleaved in itself.

On the plus side, you have set it apart with # ifdef's. I suggest going further in that direction, to wit:

1. Abstract away all the semantic code that is currently set off with # ifdefs. Replace each with a function call, prefixed with objc_.

I can do that. But there will probably be a lot of small functions with awkward names. There are also the cases where the code is very interleaved with the existing code, like this example:

#if DMD_OBJC
    bool isObjcSelector = ty == Tobjcselector || (tn && tn->ty == Tobjcselector);
#else
    bool isObjcSelector = false;
#endif
    if (tn && ty != Tfunction && tn->ty != Tfunction && ty != Tenum && !isObjcSelector)

What can I do about those cases? There are many of these cases in "mtype.c".

4. Separate out additions to dmd data structures as being structs, named with a Objc_ prefix, where the whole struct becomes a field, rather than as individual fields. This enables the struct to be abstracted away, as well as its construction, rather than having its bookkeeping implementation scattered through the dmd code.

Sure, I can do that.

By eliminating the # ifdefs, the code should become much more tractable, encapsulated, greppable. and convertible to DDMD. It should also become more resistant to requiring rebasing, and other PR's requiring rebasing.

Yeah, you're probably right. Although, I did carefully placed the #if to work properly with DDMD.

[WalterBright]

Thanks, Jacob. I don't have solutions to the problems I mentioned either, but I hope that by putting the O-C support in separate files, they can be maintained reasonably independently.

[jacob-carlborg]

Hmm, I don't understand why it tries to read a file path like "runnable/objc/objc/objc_call.d" instead of "runnable/objc/objc_call.d". It adds an extra "objc" directory, somehow [1]. I had to modify the makefile that runs the tests to add an "objc" directory [2], but my knowledge about makefiles are very limited.

[1] https://auto-tester.puremagic.com/pull.ghtml?projectid=1&runid=1158907
[2] jacob-carlborg@f3c5886

[ibuclaw]

@jacob-carlborg If it works locally, then try removing $(QUIET) from the objc test runs, to see if it is really a make problem.

[jacob-carlborg]

@ibuclaw Turns out it doesn't work locally. I had commented out all non Objective-C related tests [1], then it worked. But when I added back the rest of the tests then I got this problem.

[1] https://github.com/jacob-carlborg/dmd/blob/f3c588649b4e9bbec8211dabe55d68d18d601c35/test/Makefile#L178-L183

[jacob-carlborg]

@WalterBright Just to be clear, when you say I should abstract away the #if. If I move some code to a function and prefix it with _objc. I would move the #if DMD_OBJC inside the function and always call it? Like this:

void foo ()
{
// some code
objcFoo();
// some more code
}

void objcFoo ()
{
#if DMD_OBJC
// some code ...
#endif
}

foo(); // always call

And with the data structures, like this:

class StructDeclaration : public AggregateDeclaration
{
public:
#if DMD_OBJC
    int selectortarget;
    int isselector;
#endif
}

Replace with:

class StructDeclaration : public AggregateDeclaration
{
public:
    ObjcStructDeclaration objc;
}

struct ObjcStructDeclaration
{
    int selectortarget;
    int isselector;
}

But in this case with the data structures, where should I put the #if DMD_OBJC?

[WalterBright]

@jacob-carlborg I suggest either moving the ifdef to an implementation file, or have two implementation files, one that implements them and the other that stubs them out, and select the right one via the makefile.

For the objc struct declarations, the definition of them goes into a .h file, same as above.

If using one implementation file, there should be one ifdef in it. Having ifdef interleaved throughout is a technique I've really come to dislike over the years. I fight this battle constantly with druntime :-)

[ibuclaw]

I think %.d.out will match anything here, even directory separators, so one specific for objc is not needed. Not sure yet how you ended up with /objc/objc/

[yebblies]

If using one implementation file, there should be one ifdef in it. Having ifdef interleaved throughout is a technique I've really come to dislike over the years. I fight this battle constantly with druntime :-)

This works a lot better when it doesn't need to change data structures.

[don-clugston-sociomantic]

You've changed the return value from bool to int, yet the non-DMD_OBJC portions still return true.

[jacob-carlborg]

Right, good point.

[dnadlinger]

Please, no more boolean-named functions that return int. The ones we have at this point are confusing enough. Make it an enum if needed.

[jacob-carlborg]

I think it should return the BE enum which already exists: https://github.com/jacob-carlborg/dmd/blob/d-objc/src/statement.h#L70

[jacob-carlborg]

@WalterBright @yebblies I think I will go with an approach like this:

#if DMD_OBJC
int foo () { return 3; }
#else
int foo ()
{
    assert(0);
    return 0;
}
#endif

class StructDeclaration : public AggregateDeclaration
{
public:
    ObjcStructDeclaration objc;
};

#if DMD_OBJC
struct ObjcStructDeclaration
{
    int selectortarget;
    int isselector;
};
#else
struct ObjcStructDeclaration { };
#endif

Of course I can split up the code in multiple files.

[jacob-carlborg]

I think %.d.out will match anything here, even directory separators, so one specific for objc is not needed. Not sure yet how you ended up with /objc/objc/

@ibuclaw I commented out all changes I med to the test Makefile. It does not run the Objective-C tests.

[WalterBright]

@jacob-carlborg the code with DMD_OBJC ifdef's should be in separate .h and .c files.

[yebblies]

the code with DMD_OBJC ifdef's should be in separate .h and .c files.

When the code is adding members to classes hiding it behind a struct in another file will just make it harder to see.

[jacob-carlborg]

the code with DMD_OBJC ifdef's should be in separate .h and .c files.

@WalterBright yes, I just tried to compress the code in the above example.

[jacob-carlborg]

When the code is adding members to classes hiding it behind a struct in another file will just make it harder to see.

@yebblies what do you suggest I do instead?

[WalterBright]

@jacob-carlborg I see we're on the same page, then.

@yebblies I've done it both ways. The struct in another file works better. The # ifdef's are ugly visual noise and impair comprehension.

[jacob-carlborg]

@WalterBright @yebblies @michelf it's ready for review again. This is what I've updated:

• Abstracted away all Objective-C related code to free functions, prefixed with objc_. There's one header file, objc.h. The implementation is split across multiple files, prefixed with objc_. The filename matches the standard DMD file where the functions are used, i.e. all functions in objc_func.c are used in func.c. Looking in all files prefixed with objc_ and searching for the functions in the rest of the project should give a pretty good idea of what's been changed
• Replaced the selector syntax (void foo () [foo];) with a compiler recognized UDA. The new syntax looks like this: void foo () @selector("foo");. The UDA has some additional semantics, i.e. it can only be used once per method and only on methods with Objective-C linkage
• Replaced and removed pragma overridename with the standard mangle
• Removed the exception bridge

Some minor changed:

• Replaced Protocol* (pointer to struct) with Protocol (class) in druntime
• Replaced the base class of Protocol to NSObject to better match modern Objective-C
• Fixed casting Objective-C interface to class
• Probably some other minor things as well

There's a lot of new commits, i.e. Move implementation.... These can be squashed later if necessarybut I want keep them during the review. It makes it more flexible with rebasing.

@WalterBright Your concerns about not being able to show the diff in Github. If that's the case, I would recommend pulling the changes to your local machine and look at them. It should be much easier now when most of the implementation has been moved to the files prefixed with objc_.

Also if you have any Objective-C specific questions, just feel free to ask.

[jacob-carlborg]

I guess the tests on OS X will fail until the corresponding druntime pull request is merged.

[jacob-carlborg]

@WalterBright @yebblies @michelf ping
@ibuclaw @klickverbot how does this look like for GDC and LDC?

[michelf]

Looks good to me. I haven't tried to compile it to test it though. (I haven't compiled DMD for a few years actually, and what I had setup for that is broken.)

My question would be if the core.attribute module is the right one for an Objective-C specific UDA like @selector. Would it be better in an objc.attribute module?

Also, Swift integrates Objective-C pretty much the same we as we do and uses @objc instead of @selector as an attribute name for this purpose. Perhaps it'd be best if we used the same attribute name.

Other than this trivial non-issue, I don't really have anything to complain about.

[dnadlinger]

@jacob-carlborg: It will obviously take quite a bit of work to adapt the codegen, but from a brief glance at the code I couldn't find any conceptual issues. Also pinging @redstar.

[jacob-carlborg]

@michelf

My question would be if the core.attribute module is the right one for an Objective-C specific UDA like @selector. Would it be better in an objc.attribute module?

I have no idea. I was thinking core.attribute could be used as a module for potentially other attributes as well.

Also, Swift integrates Objective-C pretty much the same we as we do and uses @objc instead of @selector as an attribute name for this purpose. Perhaps it'd be best if we used the same attribute name.

To me @selector was a quite obvious name, since it's used to set the selector. In Swift it looks like @objc is a bit more generic, used to set the Objective-C class name as well.

Other than this trivial non-issue, I don't really have anything to complain about.

Cool.

[jacob-carlborg]

It will obviously take quite a bit of work to adapt the codegen, but from a brief glance at the code I couldn't find any conceptual issues.

@klickverbot from the the point of view of LDC and GDC I was hoping I haven't mixed the front end and codegen too much.

About the codegen, I'm not sure how much part of this is in LLVM and how much is in Clang. Perhaps you can take some parts from Clang if necessary.

[WalterBright]

9000 lines of changes, omg! I don't know how to review this.

[ghost]

[michelf]

I'm not too sure how it can be reviewed either. That's something we have to figure out.

When I started working on this, I added roughly one feature by commit. (You can see the early commits here). In principle those early commits would be easy to review independently, although given all the transformations that were made later many parts of them are no longer relevant. In particular, they were made on an older dmd codebase (from about four years ago) and were never rebased (instead the newer code base was merged in), making things harder to review still.

I wonder how much work it'd be for Jacob to recreate a fake commit history by splitting changes in one commit for each implemented Objective-C feature. That way it could be reviewed commit by commit (and if necessary, split in a couple of pull requests). That's a lot of work, but it might be what's needed to do a good review.

I'd tend to say it is less necessary to do that for the d-runtime pull request, unless the intent is to merge those changes in DMD progressively.

[jacob-carlborg]

9000 lines of changes, omg! I don't know how to review this.

@WalterBright I would start with looking in all files with its name starting with objc_. All (most) of these files have corresponding file in standard DMD without the objc_ prefix. All functions defined in the file prefix with objc_ are used in the corresponding file without the prefix. Example, every function declared in objc_e2ir.c are used in e2ir.c.

objc.h contains the declaration of all the functions in all files prefixed with objc_. objc_stubs.c contains the implementation of all functions declared in objc.h for all platforms not supporting D/Objective-C. Most of these functions either contains an assert(false) indicating they shouldn't be called on this platform, just return a default value for the given return type or are empty.

I created a new enum type, ControlFlow, which some of these helper functions return. This indicates what should happen (returning, breaking, nothing and so on) in the caller when one of these helper methods returns.

[jacob-carlborg]

When I started working on this, I added roughly one feature by commit. (You can see the early commits here). In principle those early commits would be easy to review independently, although given all the transformations that were made later many parts of them are no longer relevant. In particular, they were made on an older dmd codebase (from about four years ago) and were never rebased (instead the newer code base was merged in), making things harder to review still.

@michelf Ideally I would have used rebase to change the existing commits but that was quite difficult when you used "merge" instead of "rebase" to update latest changes from upstream.

I wonder how much work it'd be for Jacob to recreate a fake commit history by splitting changes in one commit for each implemented Objective-C feature. That way it could be reviewed commit by commit (and if necessary, split in a couple of pull requests). That's a lot of work, but it might be what's needed to do a good review.

That would probably be a lot of work. I would need to figure out which feature all of the code belongs to (I haven't fully grasped that yet).

[jacob-carlborg]

About the number of commits, the commits between 797438e and 75ca474 can be squashed the review is done. All these commits basically just moves code around.

[michelf]

@jacob-carlborg Actually, the problem was present even before the merges, as often commits were made to correct bugs in features introduced in earlier commits, meaning a review of the earlier commits by themselves wouldn't have been in their best shape for review. But it's true the merges just made things a lot worse.

My idea to construct a fake history would consist of making a list of features (in the order they were introduced), then take the current diff, remove anything from it not implementing the first feature on the list, and then make a commit out of that. Then iterate to make a new commit for each feature, and at the end you have your fake history for the same diff. I wouldn't expect more than 20 commits out of that.

But I agree it's a lot of work; if there's an easier way of doing this, such as if Walter thinks he can review the whole thing as one big diff, then I'm all for it. In the end, it's the one doing the review who should give some guidance on what's acceptable to him. What we have at this time is “9000 lines of changes, omg! I don't know how to review this.” @WalterBright, any comment?

[jacob-carlborg]

@jacob-carlborg Actually, the problem was present even before the merges, as often commits were made to correct bugs in features introduced in earlier commits, meaning a review of the earlier commits by themselves wouldn't have been in their best shape for review. But it's true the merges just made things a lot worse.

@michelf In my opinion, the correct way to handle this is to make an interactive rebase and change a previous commit. This way you get the most clean history and can avoid stupid commits like "Fix spell error". It takes a bit more work but I think it's worth it. When you're used to it, it's not a big deal.

My idea to construct a fake history would consist of making a list of features (in the order they were introduced), then take the current diff, remove anything from it not implementing the first feature on the list, and then make a commit out of that. Then iterate to make a new commit for each feature, and at the end you have your fake history for the same diff. I wouldn't expect more than 20 commits out of that.

The hard part will be to figure out which code belongs to which feature. Especially since you said you've made commits that fix bugs in previous features. In the end it will be the same amount of code anyway, just divided in chunks.

But I agree it's a lot of work; if there's an easier way of doing this, such as if Walter thinks he can review the whole thing as one big diff, then I'm all for it. In the end, it's the one doing the review who should give some guidance on what's acceptable to him

Yeah, but I already did a major refactoring when I moved most code to the files prefixed with objc_, just as Walter wanted me to. I don't feel so enthusiastic about making another major change.

[yebblies]

The hard part will be to figure out which code belongs to which feature.

And that's exactly why re-organizing it into 'feature' commits helps the review - otherwise the reviewers have to do the hard part and work out what the code's doing.

[jacob-carlborg]

@WalterBright is this the only way, to split the pull request into one for each feature?

[yebblies]

I don't think anyone wants multiple pull requests, just a way to navigate through the changes. The commit list is close to useless as-is, and the total set of changes is way too big to look at the main diff. The DIP helps, but it's difficult to what in there correlates to which changes.

eg The DIP mentions you can throw and catch Objc exceptions. So ideally there would be a commit or a small series of commits that clearly explain what was implemented and why it was implemented that way. Walter and I could crawl through the commit history and/or the diff looking for that, but I assume you already know where it is and how it works.

[jacob-carlborg]

@yebblies Ok, I see. I'll have a look and try to see how much work it would be.

@michelf If I'm going to split this pull request up, how do suggest I divide the features? Something like this:

1. Classes, instance methods and class methods (is it possible to split this as well?)
2. Interfaces

• Selector literals (__selector)
• String literals
• Properties
• Casts
• Constructors
• Destructors (not implemented yet)
• Invariant
• Synchronized
• Selector inheritance
• Non fragile instance variables

I'm thinking the first two needs to be implemented in order. For the others, I'm not sure if they have any dependencies between each other.

[michelf]

@jacob-carlborg If you look at the commit history when I started the project, I first implemented instance methods on interfaces, then class methods on interfaces, then writing class definitions in the object file, then protocol definitions in the object file, then "fixed" the "static" function's semantics by giving them a this pointer to the metaclass, then implemented access to the metaclass as an inner class and created the "magic" .class expression to get the metaclass from an instance. I'm not sure how that should be split and in what order, but there's a lot of plumbing to cover and I doubt a broad class/interface split is a useful separation line, as semantically they're almost the same thing.

[jacob-carlborg]

I'm closing this in favor of several upcoming pull requests, the first one being #4287.