================
Comment at:
clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h:129
@@ +128,3 @@
+reformatRanges(const StringRef FileName, const RangeVector &Ranges,
+ clang::SourceManager &SM,
+ const clang::format::FormatStyle &Style);
----------------
Ranges are SourceManager-independent, so you shouldn't need to pass it in here.
Also, it seems like this is more or less a reproduction of
tooling::Replacements reformat(const FormatStyle &Style, StringRef Code,
std::vector<tooling::Range> Ranges,
StringRef FileName = "<stdin>");
Defined in include/clang/Format/Format.h. Why not just use that?
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:37
@@ -35,1 +36,3 @@
+static cl::opt<std::string> FormatStyleOpt(
+ "format",
----------------
I wonder whether we can/should reuse the option from
tools/clang-format/ClangFormat.cpp. Seems bad to duplicate it (especially the
description, which has changed repeatedly). Also, I think that "style" might be
a slightly better name.
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:61
@@ +60,3 @@
+/// couldn't be parsed.
+static bool handleFormatStyle(const char *ProgName, const StringRef
OptionValue,
+ format::FormatStyle &Style,
----------------
Seems like we should pull out the getStyle()-function out of
tools/clang-format/ClangFormat.cpp and reuse it here.
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:174
@@ +173,3 @@
+bool applyFormatting(const std::vector<tooling::Replacement> &Replacements,
+ std::string &FileData,
+ const format::FormatStyle &FormatStyle,
----------------
Use a separate input and output parameter, i.e.:
StringRef InputData, std::string &OutputData,
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:192-197
@@ +191,8 @@
+
+ if (FormattingReplacements.empty())
+ return true;
+
+ Rewriter Rewrites(SM, LangOptions());
+ if (!tooling::applyAllReplacements(FormattingReplacements, Rewrites))
+ return false;
+
----------------
AFAICT, these lines are shared between applyFormatting and applyReplacements.
How about moving them into getNewDataFor (possibly renaming it to
getRewrittenData).
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:212
@@ -60,1 +211,3 @@
+ // Handle the -format option if present.
+ format::FormatStyle FormatStyle;
----------------
I think the option "-format" should be a bool option and there should be an
additional option "-style". That makes it easier to choose and change defaults.
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:255
@@ +254,3 @@
+ if (!applyReplacements(I->getValue(), NewFileData, Diagnostics)) {
+ errs() << "Failed to apply replacements to " << I->getKey() << "\n";
+ continue;
----------------
This should be done via the 'Diagnostics' (and also inside applyReplacements).
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:262
@@ +261,3 @@
+ Diagnostics)) {
+ errs() << "Failed to apply reformatting replacements for " << I->getKey()
+ << "\n";
----------------
Same here.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:267
@@ +266,3 @@
+
+ coalesceRanges(ChangedRanges);
+
----------------
I think this steps adds unnecessary complexity. Ranges will in most cases be
separated and even if not, clang-format should run fine with overlapping
ranges. If anything, it would make more sense to move this functionality into
clang-format itself as an optimization, but I doubt that it adds value.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:261
@@ +260,3 @@
+ const tooling::Replacement &R = *I;
+ unsigned Offset = tooling::shiftedCodePosition(Replaces, R.getOffset());
+ unsigned Length = R.getReplacementText().size();
----------------
AFAICT shiftedCodePosition does not work if the vector is not sorted (that
should probably go in the comment there). Also note that implemented like this,
the runtime is quadratic in the number of replacements. If we ever expect many
replacements, we should probably inline the functionality of
shiftedCodePosition here and shift all ranges in a single run through the loop.
http://llvm-reviews.chandlerc.com/D1730
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
