[cfe-dev] getLocStart versus getStartLoc

[Stephen Kelly via cfe-dev]

Richard Smith via cfe-dev wrote:

> On 26 June 2018 at 15:54, Stephen Kelly via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
> 
>> 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
> 
> 
> 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