This is an archive of the discontinued LLVM Phabricator instance.

[Rewrite] Extend to further accept CharSourceRange
ClosedPublic

Authored by jdenny on May 2 2019, 2:27 PM.

Details

Summary

Some Rewrite functions are already overloaded to accept CharSourceRange, and this extends others in the same manner. I'm calling these in code that's not ready to upstream, but I figure they might be useful to others in the meantime.

This patch also adds some unit tests. I'm not very familiar with unit testing here. I used clangTooling to build the AST. Is that appropriate?

Diff Detail

Event Timeline

jdenny created this revision.May 2 2019, 2:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 2:27 PM
jdenny added a comment.May 9 2019, 3:49 PM

Ping. This seems like a straight-forward extension to the Rewriter API.

jdenny added inline comments.May 9 2019, 3:57 PM
clang/unittests/Rewrite/RewriterTest.cpp
2

Oops. Need to change the description here.

jdenny updated this revision to Diff 207301.Jul 1 2019, 7:30 AM

Rebased, fixed a comment, and applied a clang-format suggestion.

Ping.

jdenny marked an inline comment as done.Jul 1 2019, 7:30 AM
jdenny added reviewers: jdoerfert, hfinkel.

Could you describe the unit tests a little, what is tested and why.

jdenny updated this revision to Diff 208247.Jul 5 2019, 5:55 PM

@jdoerfert Thanks for your reply. I added some comments to the tests. Let me know whether that's what you're looking for.

jdoerfert accepted this revision.Jul 5 2019, 6:09 PM

The comments help a lot. LGTM.

This revision is now accepted and ready to land.Jul 5 2019, 6:09 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 7:55 PM
thakis added a subscriber: thakis.Jul 7 2019, 8:39 AM
thakis added inline comments.
cfe/trunk/unittests/Rewrite/CMakeLists.txt
12 ↗(On Diff #208250)

This makes RewriteTests depend on clangTooling, and in follow-ups on clangFrontend and clangSerialization. Rewrite used to depend on basically only clangBasic and clangLex, and now it depends on almost all of clang. Maybe there's a less heavy-weight way to test this?

Also, when do you expect to land code that uses this? Checking in code that's unused upstream over a long period of time seems suboptimal. I delete code that shows up unused on http://llvm-cs.pcc.me.uk/ every now and then for example. (Ignore this part if you expect to check in clients of the overload soon.)

jdenny added inline comments.Jul 7 2019, 9:02 AM
cfe/trunk/unittests/Rewrite/CMakeLists.txt
12 ↗(On Diff #208250)

This makes RewriteTests depend on clangTooling, and in follow-ups on clangFrontend and clangSerialization. Rewrite used to depend on basically only clangBasic and clangLex, and now it depends on almost all of clang. Maybe there's a less heavy-weight way to test this?

Sure, I can look for another way to test this. I didn't realize this would be a real problem. But....

Also, when do you expect to land code that uses this?

Not soon. I offered this patch thinking that obvious Rewrite extensions like this one could prove useful to the larger community even if there are no upstream users now. I'm also fine to revert and keep my Rewrite extensions private for now. Is that the best approach?

jdenny added inline comments.Jul 9 2019, 8:51 AM
cfe/trunk/unittests/Rewrite/CMakeLists.txt
12 ↗(On Diff #208250)
This makes RewriteTests depend on clangTooling, and in follow-ups on clangFrontend and clangSerialization. Rewrite used to depend on basically only clangBasic and clangLex, and now it depends on almost all of clang. Maybe there's a less heavy-weight way to test this?

I'm thinking of adding more Rewrite unit tests in the future. Does anyone have advice on this issue? That is, is there any easy way to build ASTs in unit tests without such dependencies? On the other hand, these dependencies are common throughout clang/unittests subdirectories, so I'm not sure why RewriteTests needs to avoid them.