ioeric added inline comments.
================
Comment at: include/clang/Format/Format.h:862
///
/// \returns FormatStyle as specified by ``StyleName``. If no style could be
/// determined, the default is LLVM Style (see ``getLLVMStyle()``).
----------------
Is this still true?
================
Comment at: lib/Format/Format.cpp:424
+template <typename... ArgTs> llvm::Error make_string_error(ArgTs &&... Args) {
+ return llvm::make_error<llvm::StringError>(std::forward<ArgTs>(Args)...,
----------------
Why do you need a template if you only use this function with one argument?
================
Comment at: lib/Format/Format.cpp:1890
}
- FormatStyle Style = getLLVMStyle();
- Style.Language = getLanguageByFileName(FileName);
+ FormatStyle::LanguageKind Language = getLanguageByFileName(FileName);
----------------
amaiorano wrote:
> Since we won't always return a FormatStyle, we no longer construct one here.
> Rather, instances are created in local scopes below when required.
I'd probably keep the default style so that we don't need to set `Language`
repeatedly below.
================
Comment at: lib/Format/Format.cpp:1900
+ FormatStyle FallbackStyle = getNoStyle();
+ if (!getPredefinedStyle(FallbackStyleName, Language, &FallbackStyle)) {
+ return make_string_error("Invalid fallback style \"" +
----------------
Nit: prefer no curly brace around one liners. Same below.
================
Comment at: lib/Format/Format.cpp:1901
+ if (!getPredefinedStyle(FallbackStyleName, Language, &FallbackStyle)) {
+ return make_string_error("Invalid fallback style \"" +
+ FallbackStyleName.str());
----------------
amaiorano wrote:
> I am unsure whether I should be returning these error strings at all. I say
> this because some of the functions called in this one will output to errs(),
> which means the caller doesn't get the full picture of what went wrong.
>
> Maybe it's better to keep the original code that outputs to errs(), and just
> return an error code instead. Thoughts?
I think returning an `Expected` is the right approach. I think we should
consider using customized format-relayed error codes (like that in the
`tooling::Replacements` library) to provide richer information. For example,
you could use customized error codes instead of `llvm::inconvertibleErrorCode`
when constructing a `StringError` to provide additional information besides the
error message. Other interfaces in the format library can potentially benefit
from codes as well (e.g. `ParseError` can be merged).
================
Comment at: lib/Tooling/Refactoring.cpp:86
- format::FormatStyle CurStyle = format::getStyle(Style, FilePath, "LLVM");
+ llvm::Expected<format::FormatStyle> FormatStyleOrError =
format::getStyle(Style, FilePath, "LLVM");
+ if (!FormatStyleOrError) {
----------------
Is this indentation intended?
================
Comment at: lib/Tooling/Refactoring.cpp:87
+ llvm::Expected<format::FormatStyle> FormatStyleOrError =
format::getStyle(Style, FilePath, "LLVM");
+ if (!FormatStyleOrError) {
+ llvm::errs() << llvm::toString(FormatStyleOrError.takeError())
<< "\n";
----------------
amaiorano wrote:
> As I wrote above, by returning the string, the caller has the burden of
> outputting the error. More importantly, getStyle may already have written
> other errors to errs(), so I'm not sure this makes sense.
>
> If we go with only returning error code, I suppose I'd use
> llvm::ErrorOr<FormatStyle> as the return type.
I don't think `getStyle` should write to `errs()` if it returns an `Error`.
https://reviews.llvm.org/D28081
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
