This is an archive of the discontinued LLVM Phabricator instance.

Added formatAndApplyAllReplacements that works on multiple files in libTooling.
ClosedPublic

Authored by ioeric on Mar 3 2016, 7:38 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric updated this revision to Diff 49740.Mar 3 2016, 7:38 AM
ioeric retitled this revision from to Added formatAndApplyAllReplacements that works on multiple files in libTooling..
ioeric updated this object.
ioeric added reviewers: klimek, djasper.
ioeric added a subscriber: cfe-commits.
djasper added inline comments.Mar 3 2016, 10:20 AM
include/clang/Tooling/Core/Replacement.h
237 ↗(On Diff #49740)

Hm... I am not sure here. I think I would implement this entirely without FileManager or FileEntries, just based on the names of the file. I guess you are worried about different paths leading to the same FileEntry?

include/clang/Tooling/ReplacementsFormat.h
1 ↗(On Diff #49740)

I think this can happily live in Refactoring.h/cpp for now.

40 ↗(On Diff #49740)

This is probably a pre-existing issue in formatReplacements, but I think conceptually it might be wrong to hand in the Style. I think it might be a better idea to use clang::format::getStyle() with the filename stored inside of the replacement to get the style that should actually be used for any given change. Not sure whether we'd also want the capability to overwrite this. Manuel, what do you think?

ioeric updated this revision to Diff 49806.Mar 4 2016, 2:16 AM
ioeric marked 3 inline comments as done.
  • Moved formatAndApplyAllReplacements to Tooling/Refactoing; added an overloaded version of formatAndApplyAllReplacements that takes no Style; groupReplacementsByFile groups Replacements by filename instead of FileEntry.
include/clang/Tooling/Core/Replacement.h
237 ↗(On Diff #49740)

You are right. Getting rid of FileManger would make more sense for users. And yes, I was worrying about different names leading to the same entry. Do we need to worry about this case? Or we can assume Replacements for the same file always have the same file path?

ioeric updated this revision to Diff 49807.Mar 4 2016, 2:19 AM
  • removed unused forward declarations.
djasper added inline comments.Mar 4 2016, 1:56 PM
include/clang/Tooling/Core/Replacement.h
228 ↗(On Diff #49807)

This was pre-existing, but I'd remove the "InFile" suffix. It doesn't seem to add any information.

230 ↗(On Diff #49807)

I think the key type in a map is always const, so no need for "const".

235 ↗(On Diff #49807)

I also wonder whether this should really return a map. I find such maps in interfaces sometimes a bit difficult as they have some cost and the subsequent analyses might actually prefer a different container. How about instead just returning a vector<Replacements>? I mean the Replacements themselves already have the filename stored in them so the key in the map is redundant anyway.

Manuel, any thoughts?

ioeric marked an inline comment as done.Mar 6 2016, 11:58 AM
ioeric added inline comments.
include/clang/Tooling/Core/Replacement.h
228 ↗(On Diff #49807)

Will remove the "InFile" suffix.

230 ↗(On Diff #49807)

I think "const" is needed since the Entry passed to map's [] operator is of type const FileEntry * in FileToReplaces[Entry].push_back(Replace). The code didn't compile without the "const" qualifier.

235 ↗(On Diff #49807)

But I think the tradeoff of using vector<Replacements> is the insertion time since we'd need to index Replcements with the same filename for each insertion. Or maybe we can have a map from filename to vector index as a local variable and return a vector<Replacements>?

djasper added inline comments.Mar 6 2016, 12:29 PM
include/clang/Tooling/Core/Replacement.h
230 ↗(On Diff #49807)

Well, now it's a string and the strings are copied anyway. I am pretty sure it will compile after removing "const".

235 ↗(On Diff #49807)

Yes, that is what I would probably do, have a local map and return a vector to keep the interface cleaner. But lets see what Manuel thinks.

ioeric added inline comments.Mar 6 2016, 12:32 PM
include/clang/Tooling/Core/Replacement.h
230 ↗(On Diff #49807)

Ooops! I was on another git branch! Yes, it definitely compiles for string. Sorry about that.

Friendly PING.

@klimek
Hi Manuel, what do you think about the return type for
groupReplacementsByFile?
Daniel suggests that instead of "std::map<std::string, Replacements>", it
returns "std::vector<Replacements>".

klimek added inline comments.Mar 14 2016, 7:44 AM
include/clang/Tooling/Core/Replacement.h
237 ↗(On Diff #49740)

I like a map-based interface slightly better here, mainly because I think it's a bit harder to write buggy code against it, it's less work if the user wants to apply some kind of sharding (threads can just hand around the vectors without the need to loop over the vector yet again to split it, or hand indices around), and (to me at least) it is slightly easier to grasp the function's contract.

Regarding the FileEntries, I'm not overly worried about file entries leading to the same file (I believe this is not too hard to handle one level higher if necessary), and the interface would seem significantly simpler without the FileManager.

include/clang/Tooling/ReplacementsFormat.h
40 ↗(On Diff #49740)

(not sure whether that's still unresolved, as I think I talked about this with Eric already)
I believe that is what style=file is for, right? I'm fine with a convenience method with style=file, but it seems like the generic one makes sense to have, too.

ioeric updated this revision to Diff 50627.Mar 14 2016, 12:18 PM
ioeric marked 10 inline comments as done.
  • renamed calculateChangedRangesInFile to calculateChangedRanges; removed const from FileToReplacementsMap key type.
klimek added inline comments.Mar 21 2016, 6:37 AM
include/clang/Tooling/Core/Replacement.h
231 ↗(On Diff #50627)

Honestly, I'd get rid of the typedef. Daniel, what do you think?

lib/Tooling/Refactoring.cpp
90 ↗(On Diff #50627)

Just call the above with the format::getStyle("file", FilePath, "LLVM")?
(note that I think "Google" is a bad default style here)

unittests/Tooling/RefactoringTest.cpp
206 ↗(On Diff #50627)

Also use a ColumnLimit to make the test more readable?

ioeric added inline comments.Mar 21 2016, 6:48 AM
lib/Tooling/Refactoring.cpp
90 ↗(On Diff #50627)

But there could be more than one FilePath in Replaces?

unittests/Tooling/RefactoringTest.cpp
206 ↗(On Diff #50627)

We don't have access to the Style here? Any way to set ColumnLimit?

djasper added inline comments.Mar 21 2016, 6:48 AM
include/clang/Tooling/Core/Replacement.h
231 ↗(On Diff #50627)

I agree. I don't think it carries its weight and actually makes the code harder to read. Plus all users can probably use auto :-).

klimek added inline comments.Mar 21 2016, 7:01 AM
lib/Tooling/Refactoring.cpp
90 ↗(On Diff #50627)

Ah, right; In that case, we have 2 options:

  1. hand in a callback FormatStyle(StringRef), so users can hand in format::getStyle if needed; adapt the fixed-style one to call the callback-based one with a callback that just returns the fixed style
  2. refactor so the inner loop becomes trivial, something like this: for (auto &ReplacementsByFile : groupReplacementsByFile(Replaces)) { Style = ... Result = applyAllReplacements(formatReplacements(getCode(ReplacementsByFile.first, ReplacementsByFile.second, Style))) && Result; } return Result;
ioeric updated this revision to Diff 51166.Mar 21 2016, 8:11 AM
ioeric marked 8 inline comments as done.
  • removed FileToReplacementsMap typedef; refactored formatAndApplyReplacements to reduce duplicated code.
lib/Tooling/Refactoring.cpp
90 ↗(On Diff #50627)

Adding a helper function seems to do the trick too.

klimek added inline comments.Mar 21 2016, 8:23 AM
unittests/Tooling/RefactoringTest.cpp
206 ↗(On Diff #51166)

I'd rely on the general case to be covered by the other test then, and just test that the style is picked up correctly (or falls back correctly to LLVM style).

ioeric added inline comments.Mar 21 2016, 10:36 AM
unittests/Tooling/RefactoringTest.cpp
206 ↗(On Diff #51166)

You are right. Shall I remove this test case for now and add general test cases(I mean non-unit test) for format::getStyle in the future?

Just wanted to check if there are existing test cases for format::getStyle.
If there is no test for it, I'll add test cases for sure :=)

ioeric updated this revision to Diff 51433.Mar 23 2016, 9:29 AM
ioeric marked 3 inline comments as done.
  • removed format::getStyle test case from ToolingTests;

@Daniel, now that Manuel is on vacation. Could you have a look at the patch?

unittests/Tooling/RefactoringTest.cpp
206 ↗(On Diff #51433)

The test case for format::getStyle has been moved to another patch.

djasper added inline comments.Mar 29 2016, 7:46 AM
include/clang/Tooling/Refactoring.h
91 ↗(On Diff #51433)

Do you have a use case where we'd want to call this with a fixed style? Otherwise, I'd just remove this function for now.

93 ↗(On Diff #51433)

Don't copy the comment for the function. It is bound to go out of sync and makes users wonder what the actual differences are. Put this function first and replace "Instead of .." with "This function use the filename stored in the replacements to determine the appropriate style.".

Then in the comment to the other just write: "/// Same as above but use fixed style \p Style."

ioeric marked an inline comment as done.Mar 29 2016, 8:11 AM
ioeric added inline comments.
include/clang/Tooling/Refactoring.h
91 ↗(On Diff #51433)

If the .clang-format doesn't exist, it can only fall back to one default style, which is LLVM now. Shouldn't users have the flexibility to specify other styles if there is no .clang-format file?

djasper added inline comments.Mar 29 2016, 8:17 AM
include/clang/Tooling/Refactoring.h
91 ↗(On Diff #51433)

I am not sure, but I usually like to keep interfaces small until we have actual use cases. It is very hard to foresee in what ways this can possibly be used. An alternative might be to hand in the style as a string which is similar to what clang-format takes on the command line. Then people can supply "file", "Google", ... and we can hand that to getStyle().

ioeric updated this revision to Diff 51933.Mar 29 2016, 9:15 AM
  • Change the Style parameter of formatAndApplyReplacements from FormatStyle to be a Style name string.
djasper accepted this revision.Mar 29 2016, 9:20 AM
djasper edited edge metadata.

Looks good.

include/clang/Tooling/Refactoring.h
93 ↗(On Diff #51933)

See also for what? Maybe just remove?

95 ↗(On Diff #51933)

Maybe Style should default to "file".

This revision is now accepted and ready to land.Mar 29 2016, 9:20 AM
ioeric updated this revision to Diff 51936.Mar 29 2016, 9:30 AM
ioeric edited edge metadata.
  • Minor changes.
This revision was automatically updated to reflect the committed changes.