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 19700: Obj-C wrong code overloading selectors and extern(D) #9402

Merged

Conversation

jacob-carlborg
Copy link
Contributor

The issue is that if an Objective-C class contains at least one method with a body it will be considered an internally defined class. This means that for a declaration for a class that is supposed to be defined externally it won't look for this class externally when linking. Basically the internally defined class has taken precedence over the externally defined class. When some of the methods don't have a body a runtime exception will occur in the Objective-C runtime when they're called.

The fix is to require extern (Objective-C) extern for externally defined classes. For internally defined classes, extern (Objective-C) is used.

The issue is that if an Objective-C class contains at least one method
with a body it will be considered an internally defined class. This
means that for a declaration for a class that is supposed to be
defined externally it won't look for this class externally when
linking. Basically the internally defined class has taken precedence
over the externally defined class. When some of the methods don't have
a body a runtime exception will occur in the Objective-C runtime when
they're called.

The fix is to require `extern (Objective-C) extern` for externally
defined classes. For internally defined classes, `extern (Objective-C)`
is used.
@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 1, 2019

Thanks for your pull request and interest in making D better, @jacob-carlborg! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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

Auto-close Bugzilla Severity Description
19700 normal [2.085.0-beta.2] Obj-C wrong code overloading selectors and extern(D)

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 "stable + dmd#9402"

@jacob-carlborg
Copy link
Contributor Author

Doc update: dlang/dlang.org#2594.

@thewilsonator
Copy link
Contributor

Buildkite failure is unrelated. Docs have the same problem as the dlang.org PR

@jacob-carlborg
Copy link
Contributor Author

I was really hoping to get this PR included in the release, to avoid having to go through a deprecation cycle for a newly added feature. What do I need to do?

@thewilsonator
Copy link
Contributor

IDK @wilzbach @MartinNowak ?

@wilzbach wilzbach merged commit 24d3584 into dlang:stable Mar 2, 2019
@jacob-carlborg
Copy link
Contributor Author

Thanks.

@thewilsonator
Copy link
Contributor

@wilzbach don't forget the dlang.org PR

@MartinNowak
Copy link
Member

Sorry, this didn't work too well with the release schedule. It was now merged after the release has been tagged (and the PR was created after the release candidate). Please check that the necessary changes are all included on stable for 2.085.1, I guess the changelog fixups will be missing.

@MartinNowak
Copy link
Member

For the record, the tools/changed program should have long been updated to purge local changelogs right away when aggregating them and thus allowing edits of the preview changelogs in dlang.org/changelog.
The fact that people instead got accustomed to editing 2 repos is unfortunate.

@jacob-carlborg
Copy link
Contributor Author

Please check that the necessary changes are all included on stable for 2.085.1.

This is a breaking change.

@jacob-carlborg
Copy link
Contributor Author

Sorry, this didn't work too well with the release schedule. It was now merged after the release has been tagged (and the PR was created after the release candidate)

Yeah, I know it was tight, but no release had been announced so I took a chance. Would it hurt creating a new release candidate and then retag the release?

@MartinNowak
Copy link
Member

MartinNowak commented Mar 2, 2019

Too late, sorry. Beta and release dates are fixed since about a year, see D release schedule.
I can do a 2.085.1 point release a bit earlier than usual, like in 2 weeks. As I'm currently traveling, building releases is a lot of work on spotty internet, so I don't want to follow up immediately with a point release.
If you already knew it was tight, some communication on the beta/release-candidate thread would have been helpful.
I think we should generally avoid slipping our release schedule for late changes, as that creates a lot of contention, surplus communication and precedence for more last-minute changes.
You know the dates, get your work ready in time (that is the first beta) or simply wait for the next release which follows suit.
I don't want this to sound harsh, but I simply cannot afford that people push work onto me.

@jacob-carlborg
Copy link
Contributor Author

You know the dates, get your work ready in time

I do have a life outside of D.

If there's a lot of work to create a release the process is broken. It should be one click/push to create a release.

@jacob-carlborg
Copy link
Contributor Author

If you already knew it was tight, some communication on the beta/release-candidate thread would have been helpful.

I have asked for feedback on this issue long before the original implementation was done (long before the beta was started) but received sparse or none feedback at all.

The issue this PR solves came in very late. I did communicate with Sebastian. I don't know all the exact channels or procedures to use.

@jacob-carlborg jacob-carlborg deleted the 19700-objc-internal-defined-class branch March 3, 2019 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants