Page MenuHomePhabricator

clang-rename rename-all: support reading old/newname pairs from a YAML file
ClosedPublic

Authored by vmiklos on Aug 5 2016, 4:43 AM.

Diff Detail

Event Timeline

vmiklos updated this revision to Diff 66925.Aug 5 2016, 4:43 AM
vmiklos retitled this revision from to clang-rename rename-all: support reading old/newname pairs from a YAML file.
vmiklos updated this object.
vmiklos added reviewers: klimek, omtcyfz.
vmiklos added a subscriber: cfe-commits.

As a side note, this is the last feature that LibreOffice's simple clang-based rename tool (https://cgit.freedesktop.org/libreoffice/contrib/dev-tools/tree/clang/rename.cxx) supports, and clang-rename does not. (That one takes a CSV file, but in case -export-fixes outputs YAML, then here the input should be YAML as well for consistency.

omtcyfz edited edge metadata.Aug 5 2016, 6:31 AM

Still don't see general use case for that one, but since we already support multiple renamings "at once"...

Would also be nice to support at least offset + new-name in YAML input files, too.

clang-rename/tool/ClangRename.cpp
140

"Input" name doesn't make sense to me here.

docs/clang-rename.rst
70

Please move this block upwards [preferably right after line 42]. I consider information about limitations and editor more important than numerous clang-rename's terminal interface invocation examples.

test/clang-rename/ClassTestMultiByNameYAML.cpp
3

https://reviews.llvm.org/D23158 introduces simpler clang-rename invocations in tests. Please do that here, too.

vmiklos added inline comments.Aug 5 2016, 6:39 AM
clang-rename/tool/ClangRename.cpp
140

When you feed a YAML file into include-fixer, the argument name is -input, that's what I copied. What about -names?

omtcyfz added inline comments.Aug 5 2016, 6:46 AM
clang-rename/tool/ClangRename.cpp
140

Hm. That's true, include-fixer calls it the same. Well, then it'd be okay to let it be -input, so that it's more uniform across the tools.

vmiklos updated this revision to Diff 66993.Aug 5 2016, 12:33 PM
vmiklos edited edge metadata.
vmiklos marked 2 inline comments as done.

Would also be nice to support at least offset + new-name in YAML input files, too.

Done.

Please move this block upwards [preferably right after line 42]. I consider
information about limitations and editor more important than numerous
clang-rename's terminal interface invocation examples.

Makes sense, done. I wouldn't mind if rename-all wouldn't be advertised (apart
from dumping the options) at all in the documentation, but then users would
have to look at the source code for what is the wanted YAML format, and *that*
would be bad, I guess. ;-)

https://reviews.llvm.org/D23158 introduces simpler clang-rename invocations
in tests. Please do that here, too.

Done.

.cpp.rename-at.yaml? I understand it's done for the purpose of doing -input %s.rename-at.yaml, though I don't think getting proper names for them and either doing some string magic in tool invocation or just passing full name is wrong.

Other than that my concerns seem to be resolved. I'd wait for Manuel to jump in if he has time on Monday, though.

+ I'll take a look again on Monday, too. May be too sleepy now :)

Ah, and yes, it's better to move *.yaml to extra/test/clang-rename/Inputs in order to prevent confusion as every other file in extra/test/clang-rename is a test.

vmiklos updated this revision to Diff 67215.Aug 8 2016, 12:18 PM

.cpp.rename-at.yaml?

I just discovered that lit provides %S that allows getting rid of the confusing
.cpp.yaml, I'm using that now.

Ah, and yes, it's better to move *.yaml to extra/test/clang-rename/Inputs

Done.

omtcyfz added inline comments.Aug 9 2016, 7:58 AM
clang-rename/tool/ClangRename.cpp
65

AFAIK there's no need to do that, integer types are by default initialized with 0, aren't they?

vmiklos added inline comments.Aug 9 2016, 8:12 AM
clang-rename/tool/ClangRename.cpp
65

Are you sure? Here is a minimal version that shows what goes wrong when that's not initialized explicitly: http://pastebin.com/raw/2ZsUgWf6 The "Use of uninitialised value of size 8" goes away with an explicit initialization.

omtcyfz accepted this revision.Aug 9 2016, 9:31 AM
omtcyfz edited edge metadata.

LGTM

clang-rename/tool/ClangRename.cpp
65

Well, in this case - yes. But in that case the default unsigned constructor isn't called.

What I meant is:

RenameAllInfo() : Offset(0) {}
  • >
RenameAllInfo() : Offset() {}

In this case Offset is default-constructed. However, it is also true that one might argue that it's less "trivial" (whatever "triviality" definition would be :]). I guess it's fine to leave it like this.

This revision is now accepted and ready to land.Aug 9 2016, 9:31 AM
omtcyfz added inline comments.Aug 9 2016, 9:33 AM
clang-rename/tool/ClangRename.cpp
65

Well, in this case - yes. But in that case the default unsigned constructor isn't called.

<to prevent confusion>

-> Well, in the case you sent - yes. But there is no default-construction there as there is no constructor with initializer list at all.

vmiklos added inline comments.Aug 9 2016, 11:26 AM
clang-rename/tool/ClangRename.cpp
65

Ah, I see what you mean. :-) I don't mind either way, but then I'll leave it as-is for now.

This revision was automatically updated to reflect the committed changes.
alexfh added a subscriber: alexfh.Aug 9 2016, 11:55 AM

A few late comments.

clang-tools-extra/trunk/clang-rename/tool/ClangRename.cpp
65 ↗(On Diff #67381)

I'd use inline inititalizer instead.

73 ↗(On Diff #67381)

Remove the stray slash.

152 ↗(On Diff #67381)

Please use the explicit type, since it's not clear from the context.

156 ↗(On Diff #67381)

Instead, I'd return the error code. The return value of this function seems to be propagated to the main's return value anyway.

clang-tools-extra/trunk/docs/clang-rename.rst
45 ↗(On Diff #67381)

"Translation unit" is not a proper name, it shouldn't be capitalized.

A few late comments.

I've addressed these in r278201.