This is an archive of the discontinued LLVM Phabricator instance.

[clang-rename] add support for template parameter renaming
ClosedPublic

Authored by omtcyfz on Jul 27 2016, 4:52 AM.

Details

Summary

Few simple tweaks allow template parameters to be renamed. See TemplateTypenameFindBy{TemplateParam|TypeInside}.cpp for example how it works.

Diff Detail

Repository
rL LLVM

Event Timeline

omtcyfz updated this revision to Diff 65707.Jul 27 2016, 4:52 AM
omtcyfz retitled this revision from to [clang-rename] add support for template parameter renaming.
omtcyfz updated this object.
omtcyfz added reviewers: alexfh, klimek.
omtcyfz added a subscriber: cfe-commits.

Ping. Change is quite trivial.

omtcyfz updated this revision to Diff 66095.Jul 29 2016, 2:17 AM

git rebase; git resolve conflicts

alexfh requested changes to this revision.Aug 1 2016, 10:16 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-rename/USRFinder.cpp
80 ↗(On Diff #66095)

It's not common to use braces around single-line if bodies in LLVM/Clang code.

clang-rename/USRLocFinder.cpp
104 ↗(On Diff #66095)

nit: const auto *

test/clang-rename/TemplateTypenameFindByTemplateParam.cpp
1 ↗(On Diff #66095)

As noted elsewhere, this way of testing is wrong.

test/clang-rename/TemplateTypenameFindByTypeInside.cpp
1 ↗(On Diff #66095)

ditto

This revision now requires changes to proceed.Aug 1 2016, 10:16 AM
omtcyfz added inline comments.Aug 1 2016, 10:41 AM
clang-rename/USRFinder.cpp
80 ↗(On Diff #66095)

I do that literally everywhere :D LLVM Code Style isn't against it and I thinks it is good in terms of readability.

omtcyfz updated this revision to Diff 66372.Aug 1 2016, 1:38 PM
omtcyfz edited edge metadata.

Addressed comments; updated to the latest revision.

omtcyfz updated this revision to Diff 66373.Aug 1 2016, 1:39 PM
omtcyfz edited edge metadata.

Apply clang-format to fit 80 cols limit.

omtcyfz marked 3 inline comments as done.Aug 1 2016, 1:40 PM
alexfh accepted this revision.Aug 2 2016, 2:29 AM
alexfh edited edge metadata.

LG

clang-rename/USRFinder.cpp
80 ↗(On Diff #66373)

I'm not saying it's prohibited by the LLVM coding guidelines, I'm saying that the other convention seems to be substantially more popular across the LLVM project. The difference in how readable the two styles are is not large, however, mixing the styles may actually hurt readability.

It looks like the use of braces for single-line if/for/... bodies is more common in the files being changed in this patch, so being locally consistent wrt. brace usage here is probably more valuable than pursuing consistency with LLVM code in general.

However, please be careful when changing code that prevalently uses the other style (no braces for single-line if/... bodies). Introducing inconsistencies is not good in terms of readability.

This revision is now accepted and ready to land.Aug 2 2016, 2:29 AM
This revision was automatically updated to reflect the committed changes.
chapuni added a subscriber: chapuni.Aug 2 2016, 6:38 AM

Seems a couple of tests would be incompatible to -fdelayed-template-parsing. Appeased in r277452.

alexfh added a comment.Aug 2 2016, 5:09 PM

Seems a couple of tests would be incompatible to -fdelayed-template-parsing. Appeased in r277452.

Thank you! And sorry for the inconvenience.