Page MenuHomePhabricator

[libTooling] Update Transformer's `node` combinator to include the trailing semicolon for decls.
ClosedPublic

Authored by ymandel on Nov 20 2020, 8:29 AM.

Details

Summary

Currently, node only includes the semicolon for (some) statements. However, declarations have the same issue of (potentially) trailing semicolons, so node should behave the same for them.

Diff Detail

Event Timeline

ymandel created this revision.Nov 20 2020, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2020, 8:29 AM
ymandel requested review of this revision.Nov 20 2020, 8:29 AM

Drive-by question from the peanut gallery, sorry if this is an ignorant one -- not all declarations have a trailing semicolon; is that handled properly? e.g., int x; has a trailing semicolon but int x, y; only has a trailing semicolon for one of the two declarations. Relatedly, in int f(int x);, the declaration of f has a trailing semicolon, but the declaration of x does not.

ymandel edited the summary of this revision. (Show Details)Nov 20 2020, 8:42 AM
ymandel updated this revision to Diff 306707.Nov 20 2020, 8:49 AM

Clarified that semicolons are only removed if present. Fixed test.

Drive-by question from the peanut gallery, sorry if this is an ignorant one -- not all declarations have a trailing semicolon; is that handled properly? e.g., int x; has a trailing semicolon but int x, y; only has a trailing semicolon for one of the two declarations. Relatedly, in int f(int x);, the declaration of f has a trailing semicolon, but the declaration of x does not.

No, it's a good question -- the comments and the patch description should have been clearer. I've updated both. Also, I found a test that needed to be fixed (and also, hopefully, illustrates why the new behavior is preferred. The old behavior left the semicolon out of the replacement (which looks weird) because it was working around the fact that the source being removed didn't include the trailing semicolon, while the new version can just specify the replacement correctly).

Thanks!

Drive-by question from the peanut gallery, sorry if this is an ignorant one -- not all declarations have a trailing semicolon; is that handled properly? e.g., int x; has a trailing semicolon but int x, y; only has a trailing semicolon for one of the two declarations. Relatedly, in int f(int x);, the declaration of f has a trailing semicolon, but the declaration of x does not.

No, it's a good question -- the comments and the patch description should have been clearer. I've updated both. Also, I found a test that needed to be fixed (and also, hopefully, illustrates why the new behavior is preferred. The old behavior left the semicolon out of the replacement (which looks weird) because it was working around the fact that the source being removed didn't include the trailing semicolon, while the new version can just specify the replacement correctly).

Thanks!

Thanks, this is more clear now!

tdl-g accepted this revision.Nov 20 2020, 9:05 AM
This revision is now accepted and ready to land.Nov 20 2020, 9:05 AM
Harbormaster completed remote builds in B79618: Diff 306707.
This revision was landed with ongoing or failed builds.Nov 20 2020, 10:12 AM
This revision was automatically updated to reflect the committed changes.