Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

This revision was automatically updated to reflect the committed changes. Closed by commit rL264745: Added formatAndApplyAllReplacements that works on multiple files in libTooling. (authored by ioeric). Changed prior to commit: http://reviews.llvm.org/D17852?vs=51936=51939#toc Repository:

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric updated this revision to Diff 51936. ioeric added a comment. - Minor changes. http://reviews.llvm.org/D17852 Files: include/clang/Basic/SourceManager.h include/clang/Format/Format.h include/clang/Tooling/Core/Replacement.h include/clang/Tooling/Refactoring.h

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Comment at: include/clang/Tooling/Refactoring.h:93 @@ +92,3 @@ +/// +/// See also "clang/Tooling/Core/Replacements.h". +bool formatAndApplyAllReplacements(const

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric updated this revision to Diff 51933. ioeric added a comment. - Change the Style parameter of formatAndApplyReplacements from FormatStyle to be a Style name string. http://reviews.llvm.org/D17852 Files: include/clang/Basic/SourceManager.h include/clang/Format/Format.h

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

djasper added inline comments. Comment at: include/clang/Tooling/Refactoring.h:91 @@ +90,3 @@ + Rewriter , + const format::FormatStyle ); + ioeric wrote: > djasper wrote: > > Do you have a use

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric marked an inline comment as done. Comment at: include/clang/Tooling/Refactoring.h:91 @@ +90,3 @@ + Rewriter , + const format::FormatStyle ); + djasper wrote: > Do you have a use case where

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

djasper added inline comments. Comment at: include/clang/Tooling/Refactoring.h:91 @@ +90,3 @@ + Rewriter , + const format::FormatStyle ); + Do you have a use case where we'd want to call this

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric added a comment. @Daniel, now that Manuel is on vacation. Could you have a look at the patch? Comment at: unittests/Tooling/RefactoringTest.cpp:206 @@ -169,3 +205,3 @@ TEST(ShiftedCodePositionTest, FindsNewCodePosition) { Replacements Replaces;

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric updated this revision to Diff 51433. ioeric marked 3 inline comments as done. ioeric added a comment. - removed format::getStyle test case from ToolingTests; http://reviews.llvm.org/D17852 Files: include/clang/Basic/SourceManager.h include/clang/Format/Format.h

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

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 :=) On Mon, Mar 21, 2016, 21:20 Manuel Klimek wrote: > On Mon, Mar 21, 2016 at 6:36 PM Eric Liu wrote: > >> ioeric added

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

On Mon, Mar 21, 2016 at 6:36 PM Eric Liu wrote: > ioeric added inline comments. > > > Comment at: unittests/Tooling/RefactoringTest.cpp:206 > @@ +205,3 @@ > +TEST_F(ReplacementTest, ReplaceAndFormatNoStyle) { > + std::string Code =

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric added inline comments. Comment at: unittests/Tooling/RefactoringTest.cpp:206 @@ +205,3 @@ +TEST_F(ReplacementTest, ReplaceAndFormatNoStyle) { + std::string Code = "MyType012345678901234567890123456789 *a =\n" + "new

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

klimek added inline comments. Comment at: unittests/Tooling/RefactoringTest.cpp:206 @@ +205,3 @@ +TEST_F(ReplacementTest, ReplaceAndFormatNoStyle) { + std::string Code = "MyType012345678901234567890123456789 *a =\n" + "new

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric updated this revision to Diff 51166. ioeric marked 8 inline comments as done. ioeric added a comment. - removed FileToReplacementsMap typedef; refactored formatAndApplyReplacements to reduce duplicated code. http://reviews.llvm.org/D17852 Files: include/clang/Basic/SourceManager.h

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric added inline comments. Comment at: lib/Tooling/Refactoring.cpp:90 @@ +89,3 @@ + +// FIXME: duplicated code here. Any better way to overload? +bool formatAndApplyAllReplacements(const Replacements , klimek wrote: > ioeric wrote: > > klimek wrote: > > > Just

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

klimek added inline comments. Comment at: lib/Tooling/Refactoring.cpp:90 @@ +89,3 @@ + +// FIXME: duplicated code here. Any better way to overload? +bool formatAndApplyAllReplacements(const Replacements , ioeric wrote: > klimek wrote: > > Just call the above with

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric added inline comments. Comment at: lib/Tooling/Refactoring.cpp:90 @@ +89,3 @@ + +// FIXME: duplicated code here. Any better way to overload? +bool formatAndApplyAllReplacements(const Replacements , klimek wrote: > Just call the above with the

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

djasper added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:231 @@ +230,3 @@ +typedef std::map +FileToReplacementsMap; + klimek wrote: > Honestly, I'd get rid of the typedef. Daniel, what do you think? I

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

klimek added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:231 @@ +230,3 @@ +typedef std::map +FileToReplacementsMap; + Honestly, I'd get rid of the typedef. Daniel, what do you think?

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

PING On Mon, Mar 14, 2016 at 8:18 PM Eric Liu wrote: > ioeric updated this revision to Diff 50627. > ioeric marked 10 inline comments as done. > ioeric added a comment. > > - renamed calculateChangedRangesInFile to calculateChangedRanges; removed > const from

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric updated this revision to Diff 50627. ioeric marked 10 inline comments as done. ioeric added a comment. - renamed calculateChangedRangesInFile to calculateChangedRanges; removed const from FileToReplacementsMap key type. http://reviews.llvm.org/D17852 Files:

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

klimek added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:237 @@ +236,3 @@ +/// related to the same file entry are put into the same vector. +FileToReplacementsMap groupReplacementsByFile(const Replacements , +

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

Friendly PING. @klimek Hi Manuel, what do you think about the return type for groupReplacementsByFile? Daniel suggests that instead of "std::map", it returns "std::vector". On Sun, Mar 6, 2016 at 9:32 PM Eric Liu wrote: > ioeric added inline

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:230 @@ +229,3 @@ + +typedef std::map +FileToReplacementsMap; djasper wrote: > ioeric wrote: > > djasper wrote: > > > I think the key type in a map is always const, so no need

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

djasper added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:230 @@ +229,3 @@ + +typedef std::map +FileToReplacementsMap; ioeric wrote: > djasper wrote: > > I think the key type in a map is always const, so no need for "const". > I

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric marked an inline comment as done. Comment at: include/clang/Tooling/Core/Replacement.h:228 @@ -226,3 +227,3 @@ /// \pre Replacements must be for the same file. -std::vector -calculateChangedRangesInFile(const tooling::Replacements ); +std::vector

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

djasper added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:228 @@ -226,3 +227,3 @@ /// \pre Replacements must be for the same file. -std::vector -calculateChangedRangesInFile(const tooling::Replacements ); +std::vector calculateChangedRangesInFile(const

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric updated this revision to Diff 49807. ioeric added a comment. - removed unused forward declarations. http://reviews.llvm.org/D17852 Files: include/clang/Basic/SourceManager.h include/clang/Format/Format.h include/clang/Tooling/Core/Replacement.h

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:237 @@ +236,3 @@ +/// related to the same file entry are put into the same vector. +FileToReplacementsMap groupReplacementsByFile(const Replacements , +

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

ioeric updated this revision to Diff 49806. ioeric marked 3 inline comments as done. ioeric added a comment. - Moved formatAndApplyAllReplacements to Tooling/Refactoing; added an overloaded version of formatAndApplyAllReplacements that takes no Style; groupReplacementsByFile groups Replacements

Re: [PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

djasper added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:237 @@ +236,3 @@ +/// related to the same file entry are put into the same vector. +FileToReplacementsMap groupReplacementsByFile(const Replacements , +