Page MenuHomePhabricator

do not merge - [clang] fix EndLoc of some Statements

Authored by AlexanderLanin on Mar 12 2020, 3:58 PM.



At the moment statements like break, continue, return, goto do not include the semicolon in the end within getEndLoc(). Add missing SourceLocation to those statements to express their EndLoc.

Warning: this is a proof of concept. It works at least in some cases. However I have no idea what I'm doing here.

Diff Detail

Event Timeline

AlexanderLanin created this revision.Mar 12 2020, 3:58 PM

Adding potentially relevant reviews.

We don't generally consider the semicolon to be part of the statement's source range. Among other things, it's really unclear how that would apply to expressions given that we don't have a separate ExprStmt class.

Thanks for the feedback. I was trying to start a discussion on cfe-dev where I referenced this commit, but I'm also fine having it here! There you'll also find some rationale. Just worried it will get out of sync.

The problem from clang-tidy checkers point of view is that Stmt.getEndLoc does include the semicolon in most cases. It's just these few where it is not.
Me and @ymandel had a mail thread over here: & but started a new one for Stmt modification.

The alternative is adding a Stmt traversal function to clang-tidy like this one: (It needs to be extended a little with GotoStmt etc).
Idea of this commit was to reduce different behavior cases to "Expr vs rest" instead of "half a dozen apparently random statements vs rest".

It would also be interesting to think about always excluding the semicolon, but that change would probably be bigger than this one.

jdenny added a subscriber: jdenny.Mar 14 2020, 7:41 AM

One more thought: This originated for me from attempting to fix braces-around-statements which has quite some trouble finding the correct place to place the }. However this is a good definition of "end location of statement" isn't it? The place where you can end the scope. The place where you could place the next statement!

AlexanderLanin abandoned this revision.Apr 24 2020, 11:45 PM

Mailing list was clear about AST having to many users. No change to clang AST.
Will hopefully be resolved by D75813 instead.