API Removal Notice (4 weeks): getStartLoc, getLocStart, getLocEnd

Clang Frontend
1

Hello,

In order to increase consistency and learnability of AST APIs, I have committed the following transformations on AST Node APIs:

  getLocStart -> getBeginLoc
  getStartLoc -> getBeginLoc
  getLocEnd -> getEndLoc

This is consistent with other SourceLocation accessors, which follow a pattern of get.*Loc.

The rationale for this change can be reviewed here:

http://clang-developers.42468.n3.nabble.com/getLocStart-versus-getStartLoc-td4061010.html

I have already ported the clang and clang-tools-extras code to use the new method names.

All third-party code must be similarly updated.

Currently, the old names remain in the code, but annotated as deprecated. I will remove the old names by committing

  ⚙ D50353 Remove deprecated API

on or after 6th September 2018. Please subscribe to that change for notifications. I will also ping this mailing list again 1 week prior to committing the removal.

Please let me know if that timeframe does not work for you or causes problems so that I can postpone.

On systems with git and sed, porting may be as simple as

  git grep -l getLocEnd | xargs sed -i 's/getLocEnd/getEndLoc/g'
  git grep -l getLocStart | xargs sed -i 's/getLocStart/getBeginLoc/g'
  git grep -l getStartLoc | xargs sed -i 's/getStartLoc\b/getBeginLoc/g'

The getStartLocation API is distinct and should be excluded from porting as above.

Because all before and after spellings are the same length, this has a neutral impact on the style of your code.

Thanks,

Stephen.

2

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

3

Thank you! This has been maddening for years.

-Chris

4

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.

5

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

6

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.

7

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.

8

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?

9

Cool, sounds good.

Yes, one of the commits had a mistake and I fixed it with r339379.

Thanks,

Stephen.

10

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

11

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?

12

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.

13

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?

I’m strongly opposed to setting a precedent that we will give out-of-tree consumers of the Clang AST such a huge amount of time to respond to API changes – given that we recently branched Clang 7, we’re talking about a year of delay if we don’t remove these until Clang 9 is branched. The four week delay we’ve been talking about for this patch is already extraordinary and goes far beyond what I’d expect from someone contributing an API cleanup. We make major breaking changes to our C++ API all the time, and we explicitly have no stability guarantees for it. And that’s a good thing, both for our own code health and for our community, as it encourages people to contribute their changes upstream (or to use the designed-for-stability C API when applicable).

I consider this particular change to be an experiment; I would encourage anyone who derived material benefit from the delay between deprecation and removal in this case to let us know.

14

For what it's worth, as a downstream consumer of the Clang C++ API, I
was happy to see the deprecation announcement (because it makes it
easier to understand why our code would break), and now I'm eager to
get rid of the old API. For us it would actually be better if removal
was backported to the 7.0 branch, because we typically do development
on master (which is synced with LLVM/Clang trunk) during the LLVM
release period. We only take a release branch of our own when an API
break is introduced between LLVM/Clang trunk and release branch, and
then we have to cherry-pick our changes carefully between the two.

- Kim

15

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.

I've merged the new API to the 7.0 branch in r340332 per the original plan.

Will you write something for the release notes?

Thanks,
Hans

16

I think this topic is worthy of a new thread with a new subject to attack a wider/different audience.

Personally, I both like and have serious hesitations about the implicit proposal of using deprecation and backporting deprecations. I definitely do not see consensus here and if we want to propose such a thing, it deserves some serious discussion.

Philip

17

Thanks for that.

I wrote ⚙ D51069 Notify pending API removal in the release notes but I didn't seem to have permissions to commit it to the release branch in the svn repo (I usually use git, so I may have done something wrong).

Thanks,

Stephen.

18

As indicated below, this is a reminder that I will remove the old API next week.

Thanks,

Stephen.

19

Completed with SVN revision 341573.

Thanks,

Stephen