Some functions were removed from SPI in r207149, but we need the declarations in headers so that they would match exports.
Created attachment 302814 [details] proposed patch
Isn’t there a sharper instrument that can be used when a symbol needs to be visible solely for binary compatibility or for use by another target in the same project? Something like declaring it in an “extra private header” that is really just an project header?
There is one, but I just got an r- today for using a nearly identical instrument. -extra-private-header is an out-of-band signal, most contributors won't know about it, it doesn't follow the code across renames and refactoring, etc.
Is there no mechanism that allows an entire directory to be designated? If you specify an -extra-private-header, is that header allowed include other headers that aren’t private and aren’t themselves designated as -extra-private-header? I see the similarities to bug 168878, but I also see differences. If we must declare these (and a few more) in a framework header, the rather than add a comment, can we annotate them with __attribute__((unavailable)) or similar?
> Is there no mechanism that allows an entire directory to be designated? A directory can be provided, yes. > If we must declare these (and a few more) in a framework header, the rather than add a comment, can we annotate them with __attribute__((unavailable)) or similar? I'm not sure. I don't see __attribute__((unavailable)) anywhere in WebKit2, and I don't know how it would interact with InstallAPI. Not sure if this is worth creating a whole new mechanism for at this time. We've been living with "deprecated" comments for years, with no problems that I'm aware of.
(In reply to comment #5) > > Is there no mechanism that allows an entire directory to be designated? > > A directory can be provided, yes. That seems like a good path going forward. The declarations in this patch used to be in an SPI header, so I see how it’s tempting to just put them back. But other internal symbols which need to be visible were never declared in a framework header, and unnecessarily publishing internal details in framework headers has no precedent. > > If we must declare these (and a few more) in a framework header, the rather than add a comment, can we annotate them with __attribute__((unavailable)) or similar? > > I'm not sure. I don't see __attribute__((unavailable)) anywhere in WebKit2, > and I don't know how it would interact with InstallAPI. We use the NS_UNAVAILABLE macro, which expands to that attribute, throughout the project. Can you check the InstallAPI angle? Since we’re talking about declarations that were either deprecated or never intended to be adopted, it would be good to make it practically impossible for clients to use. A comment does not accomplish that as well as a compiler-readable annotation. > Not sure if this is worth creating a whole new mechanism for at this time. > We've been living with "deprecated" comments for years, with no problems > that I'm aware of. The mechanism for annotating declarations as deprecated or unavailable are widely deployed in the Modern WebKit API (private declarations included). But it still seems best to never introduce those declarations in the first place.
NS_UNAVAILABLE is indeed used for three Objective-C methods in WebKit2. But I doubt that it can be used in this cross-platform C header. I can't verify what happens with __attribute__((unavailable)) until all other errors are resolved, as the tbd file can't be generated because of those. > A comment does not accomplish that as well as a compiler-readable annotation. I think that in practice, it did accomplish that.
(In reply to comment #7) > NS_UNAVAILABLE is indeed used for three Objective-C methods in WebKit2. But > I doubt that it can be used in this cross-platform C header. > > I can't verify what happens with __attribute__((unavailable)) until all > other errors are resolved, as the tbd file can't be generated because of > those. > > > A comment does not accomplish that as well as a compiler-readable annotation. > > I think that in practice, it did accomplish that. NS_UNAVAILABLE is just __attribute__((unavailable)). Use it! It must work with InstallAPI...
Created attachment 303010 [details] patch for landing
Comment on attachment 303010 [details] patch for landing Clearing flags on attachment: 303010 Committed r213198: <http://trac.webkit.org/changeset/213198>
All reviewed patches have been landed. Closing bug.
*** Bug 168995 has been marked as a duplicate of this bug. ***
(In reply to comment #8) > NS_UNAVAILABLE is just __attribute__((unavailable)). Use it! It must work > with InstallAPI... See bug #169243
For the record, __attribute__((unavailable)) has a different meaning, and should not be used like this, see rdar://problem/31139070. We will soon be able to remove these symbols altogether (in fact, I believe that they are already unnecessary, but there is also _WKVisitedLinkProvider which still is).