[cfe-dev] API Removal Notice (4 weeks): getStartLoc, getLocStart, getLocEnd

[Stephen Kelly via cfe-dev]

Hi,

I'm generally fine with that, but

* That is not how cfe generally operates regarding API changes
* There is no consensus for doing that
* There hasn't been any discussion which you might expect for such a 
change in how cfe operates.

I am not really comfortable with using a lack of consensus (and lack of 
discussion) to create new precedent of policy about how Clang does API 
removals. Currently the policy is 'just do it'. I come from experience 
in projects which are very conservative (Qt, CMake) so I wanted to at 
least be structured about the removal and notify (with this thread) 
about it.

The suggestion is to change existing policy quite dramatically, and I 
don't see consensus for doing that.

I'm inclined to stick with the plan I notified already. There is already 
implicit consensus (not unanimous approval, which is a different and 
not-sought-after thing) to do that. Thoughts?

By the way, I still think that regardless of when the old API gets 
removed, it makes sense to cherry-pick the new API to 7.0.0 if it's not 
already too late.

Thanks,

Stephen.



On 14/08/18 09:43, Hans Wennborg via cfe-dev wrote:
> I'd be fine with that too, and if we want to deprecate and remove an
> API across a release, this would really be the way to do it.
> 
> Richard, do you have any thoughts on this?
> 
> On Mon, Aug 13, 2018 at 11:40 AM, David Chisnall
> <David.Chisnall at cl.cam.ac.uk> wrote:
>> Is there a big binary-size win from removing the functions?  It seems as if the simplest solution that is friendly to downstream is to defer the commit that removes the old functions to immediately after 9.0 is branched.  That way, users of 8.x get the deprecation notice and can then upgrade everything and have it working in time for 9.0.
>>
>> Adding the new function to 7.0, without the deprecation notice and then not including the deprecation notice in 8.0 means that there is no release where downstream users that jump from release to release will ever see the deprecation.
>>
>> David
>>
>>> On 13 Aug 2018, at 10:21, Stephen Kelly <steveire at gmail.com> wrote:
>>>
>>> Cool, sounds good.
>>>
>>> Yes, one of the commits had a mistake and I fixed it with r339379.
>>>
>>> Thanks,
>>>
>>> Stephen.
>>>
>>>
>>> On Mon, 13 Aug 2018, 10:06 Hans Wennborg, <hans at chromium.org> wrote:
>>> I think David suggested merging also the deprecation notice.
>>>
>>> Apologies, I misread your statement.  Merging just the new API,
>>> without the deprecation markers, and encouraging folks to upgrade in a
>>> release note sounds good to me.
>>>
>>> Looks like that would be r339372, r339373 and r339374. Did I miss anything?
>>>
>>>
>>> On Mon, Aug 13, 2018 at 10:52 AM, Stephen Kelly <steveire at gmail.com> wrote:
>>>> Hi Hans,
>>>>
>>>> I suggested merging only the new API to 7.0.0. Not doing the port and not
>>>> deprecating the old API. Did you miss that?
>>>>
>>>> Thanks,
>>>>
>>>> Stephen.
>>>>
>>>>
>>>> On Mon, 13 Aug 2018, 09:48 Hans Wennborg, <hans at chromium.org> wrote:
>>>>>
>>>>> Hmm, if we merge the deprecation notices to 7.0.0, we'd also need to
>>>>> merge all the clang, clang-tools-extra, etc. updates to avoid the
>>>>> warnings in our own code, at which point we would have merged the
>>>>> whole thing, and I'd prefer not to do that.
>>>>>
>>>>> I think just having a clear release note for 8.0.0 explaining what's
>>>>> changed and what developers need to do is good enough. I believe
>>>>> that's how API changes normally go.
>>>>>
>>>>> On Fri, Aug 10, 2018 at 7:35 PM,  <paul.robinson at sony.com> wrote:
>>>>>> If the totality of the change is deemed too large to backported, it
>>>>>> could
>>>>>> make sense to only backport the new API and made a message to the
>>>>>> release
>>>>>> notes to encourage downstreams to port.
>>>>>>
>>>>>>
>>>>>>
>>>>>> This is a lot kinder to downstream consumers than usual, and I fully
>>>>>> support
>>>>>> doing it that way.
>>>>>>
>>>>>> --paulr
>>>>>>
>>>>>>
>>>>>>
>>>>>> From: cfe-dev [mailto:cfe-dev-bounces at lists.llvm.org] On Behalf Of
>>>>>> Stephen
>>>>>> Kelly via cfe-dev
>>>>>> Sent: Friday, August 10, 2018 1:19 PM
>>>>>> To: David Chisnall; Hans Wennborg
>>>>>> Cc: Clang Dev
>>>>>> Subject: Re: [cfe-dev] API Removal Notice (4 weeks): getStartLoc,
>>>>>> getLocStart, getLocEnd
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>>
>>>>>>
>>>>>> I think that's a release manager decision (cc Hans)
>>>>>>
>>>>>>
>>>>>>
>>>>>> The API doesn't exist in 7.0.0. All of my commits (introduce the new
>>>>>> API,
>>>>>> port to it, deprecate the old API) would have to be backported.
>>>>>>
>>>>>>
>>>>>>
>>>>>> In discussions so far, it was not a priority to keep the old API or
>>>>>> notify
>>>>>> particularly sensitively.
>>>>>>
>>>>>>
>>>>>>
>>>>>> If the totality of the change is deemed too large to backported, it
>>>>>> could
>>>>>> make sense to only backport the new API and made a message to the
>>>>>> release
>>>>>> notes to encourage downstreams to port.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>
>>>>>>
>>>>>> Stephen.
>>>>>>
>>>>>> On Fri, 10 Aug 2018, 14:44 David Chisnall, <David.Chisnall at cl.cam.ac.uk>
>>>>>> wrote:
>>>>>>
>>>>>> On 10 Aug 2018, at 00:13, Stephen Kelly via cfe-dev
>>>>>> <cfe-dev at lists.llvm.org>
>>>>>> wrote:
>>>>>>>
>>>>>>> All third-party code must be similarly updated.
>>>>>>>
>>>>>>> Currently, the old names remain in the code, but annotated as
>>>>>>> deprecated.
>>>>>>
>>>>>> If you haven’t already, please will you add this change to the set to be
>>>>>> backported to the 7.0 branch?  The deprecation notice was really useful
>>>>>> - it
>>>>>> told me exactly what I needed to change to make it go away - but if it
>>>>>> never
>>>>>> ships in a major release then a lot of downstream users will never see
>>>>>> it.
>>>>>>
>>>>>> David
>>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>