getLocStart versus getStartLoc

Hello,

Some AST classes have non-pure methods called getStartLoc, which return the
same thing as getLocStart (DeclStmt and CXXNewExpr).

This is confusing to a newcomer to the clang AST trying to write an AST
matcher for example.

Is there some reason for that?

If not, is there any objection to me

* Removing and porting away from getStartLoc() methods
* Renaming DeclStmt::setStartLoc to getStartLoc

Thanks,

Stephen.

There is no reason for this other than a change in preference at some point
in the distant past that we never cleaned up after. Please feel free to do
the cleanup you're suggesting!

Thanks, I looked into this a bit. It turns out to be a bigger job than it
first appears.

1) DeclarationNameInfo has both getLocEnd and getEndLoc. Both are used by
calling code, and I can't tell if any use is incorrect. At any rate, if the
two methods are to remain, the getLocEnd() one should be renamed to
something more specific as it is the one doing something different to other
getLocEnd/getEndLoc methods which generally don't 'try again' in the invalid
case.

2) While renaming methods from getStartLoc to getLocStart (as the more-
commonly used spelling, and as the spelling used in clang::Stmt), I realized
that the getStartLoc spelling is more conventional, (getExprLoc,
getEllipsisLoc, getUsingLoc), so getLocStart should be ported to that
everywhere.

Then there are methods which use 'getBeginLoc' instead of 'getStartLoc', and
SourceRange uses 'getBegin'. 'Begin' makes more sense as a counter-point to
'End', so that would mean porting everything to getBeginLoc.

However, that will certainly break all code external to the llvm repos, and
would need coordination if it is to be done at all (and possibly a multi-
release plan involving deprecation warnings prior to eventual removal).

Is that something we would really want to do? I value consistency, but
breaking all Clang API users so dramatically is something to do only if it
is seen as valuable to a consensus of the community.

Additionally, maybe it is only valuable to make the getLoc etc methods
consistent if it is part of a larger API review of Clang API to aim for
greater consistency throughout (I am not aware of other API candidates, but
maybe there are other blemishes).

Thoughts?

Thanks,

Stephen

1) DeclarationNameInfo has both getLocEnd and getEndLoc. Both are used by
calling code, and I can't tell if any use is incorrect. At any rate, if
the two methods are to remain, the getLocEnd() one should be renamed to
something more specific as it is the one doing something different to
other getLocEnd/getEndLoc methods which generally don't 'try again' in the
invalid case.

I created a patch to inline getLocEnd() into callers.

https://reviews.llvm.org/D49100

<snip>
Is that something we would really want to do? I value consistency, but
breaking all Clang API users so dramatically is something to do only if it
is seen as valuable to a consensus of the community.

This is still pending discussion of some sort.

Thanks,

Stephen.

  1. DeclarationNameInfo has both getLocEnd and getEndLoc. Both are used by
    calling code, and I can’t tell if any use is incorrect. At any rate, if
    the two methods are to remain, the getLocEnd() one should be renamed to
    something more specific as it is the one doing something different to
    other getLocEnd/getEndLoc methods which generally don’t ‘try again’ in the
    invalid case.

I created a patch to inline getLocEnd() into callers.

https://reviews.llvm.org/D49100

Does the “start is valid but end is not” case ever actually happen? If so, fixing getEndLoc to always return a valid location seems like the right thing. We don’t want getSourceRange() to ever return something different from SourceRange(getStartLoc(), getEndLoc()).

Is that something we would really want to do? I value consistency, but breaking all Clang API users so dramatically is something to do only if it is seen as valuable to a consensus of the community.

This is still pending discussion of some sort.

We have historically taken the position that the churn caused for out-of-tree users of the C++ API are not a consideration when performing an API refactoring. (For two reasons: one is that we explicitly do not have a stable C++ API; the other is that this is the mechanism by which we encourage people to upstream changes.) This actually seems like a relatively easy change in that regard: client code can be largely updated by a regular expression.

> 1) DeclarationNameInfo has both getLocEnd and getEndLoc. Both are used
> by calling code, and I can't tell if any use is incorrect. At any rate,
> if the two methods are to remain, the getLocEnd() one should be renamed
> to something more specific as it is the one doing something different
> to other getLocEnd/getEndLoc methods which generally don't 'try again'
> in
the
> invalid case.

I created a patch to inline getLocEnd() into callers.

https://reviews.llvm.org/D49100

Does the "start is valid but end is not" case ever actually happen?

I don't know.

If so,
fixing getEndLoc to always return a valid location seems like the right
thing. We don't want getSourceRange() to ever return something different
from SourceRange(getStartLoc(), getEndLoc()).

Yes, I started out trying to make a NFC change, but I came to the same
conclusion as you a while ago without the time to change the approach. I've
updated the Phab review now.

<snip>
> Is that something we would really want to do? I value consistency, but
> breaking all Clang API users so dramatically is something to do only if
it
> is seen as valuable to a consensus of the community.

This is still pending discussion of some sort.

We have historically taken the position that the churn caused for
out-of-tree users of the C++ API are not a consideration when performing
an API refactoring. (For two reasons: one is that we explicitly do not
have a stable C++ API; the other is that this is the mechanism by which we
encourage people to upstream changes.) This actually seems like a
relatively easy change in that regard: client code can be largely updated
by a regular expression.

Great, I'll look into doing this in a bit.

Thanks,

Stephen.

Can I commit this now? I have other things I want to get done in the clang
codebase related to other AST issues and related to Clang.

However, all of the changes I want to make will need to be reviewed, so I
can't really justify working on them until I can estimate whether they will
be reviewed.

Can I just go ahead and commit that, or is there some issue with it?

Thanks,

Stephen.

Yes, go ahead. (Sorry, I missed that you’d updated the patch.)

Great, thanks. Sorry, I'm not so familiar with Phab either, so I don't
know my way around it.

Stephen.

I've pushed some Phab reviews to complete the port. For convenience, the
commits can be seen together on my github:

https://github.com/steveire/clang/commits/API-cleanup

https://github.com/steveire/clang-tools-extra/commits/API-cleanup

My intention is to push all except the actual removal of the old API at
first, then push the removal of the old API 3 or so weeks later.

Thanks,

Stephen.

(Explicitly adding a few more people for visibility.)