Some minor comment but looks good to me.
For the changed range tests what have you planned? Will they stay in
clang-modernize?
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:328-330
@@ +327,5 @@
+
+void reformatRanges(const llvm::StringRef FileName, const RangeVec &Ranges,
+ clang::SourceManager &SM, clang::format::FormatStyle Style,
+ std::vector<tooling::Replacement> &Replacements) {
+ const clang::FileEntry *Entry = SM.getFileManager().getFile(FileName);
----------------
Edwin Vane wrote:
> Guillaume Papin wrote:
> > nit: I think we can get rid of the extra qualifications `clang::` and
> > `llvm::`.
> Actually, there's this bug in doxygen that causes it not to be able to match
> a function definition with it's implementation if we don't fully spell out
> the namespaces even though there's a using declaration. You should find that
> only the function prototypes use unnecessary namespaces. Other code, or file
> local functions should not have unnecessary names.
I'm aware of this issue but I thought it applied only to overloaded functions
where Doxygen was confused in which one to pick-up. I won't point this in the
future.
================
Comment at:
clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h:127
@@ +126,3 @@
+void reformatRanges(const StringRef FileName, const RangeVec &Ranges,
+ clang::SourceManager &SM, clang::format::FormatStyle Style,
+ std::vector<clang::tooling::Replacement> &Replacements);
----------------
`Style` should be const-ref.
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:165-170
@@ -60,1 +164,8 @@
+ // Handle the -format-style option if present.
+ format::FormatStyle FormatStyle;
+ int FormatOptResult = handleFormatStyle(argv[0], FormatStyle, Diagnostics);
+ if (FormatOptResult < 0)
+ return 1;
+ bool DoFormat = FormatOptResult > 0;
+
----------------
What about checking the option here and send the option value as argument to
`handleFormatStyle()`?
Return false on error as in the previous version.
```
if (FormatStyleOpt.getNumOccurrences() > 0) {
if (!handleFormatStyle(argv[0], FormatStyleOpt, FormatStyle, Diagnostics))
return 1;
DoFormat = true;
}
```
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:40
@@ +39,3 @@
+ cl::desc(
+ "Enable formatting of code code changed by applying replacements.\n"
+ "<style> can be either a builtin style supported by clang-format,\n"
----------------
code code
================
Comment at: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp:66-68
@@ +65,5 @@
+
+ std::string OptValue = FormatStyleOpt;
+ if (OptValue.empty())
+ OptValue = "llvm";
+
----------------
No big deal but I think a StringRef is enough here, in both case the lifetime
of the string assigned will outlive this variable.
http://llvm-reviews.chandlerc.com/D1730
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
