amaiorano added a comment.
Thank you, @ioeric for your feedback!
================
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()``).
----------------
ioeric wrote:
> Is this still true?
No, it's not true, and I intend to update this comment. Thanks for pointing it
out :)
================
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)...,
----------------
ioeric wrote:
> Why do you need a template if you only use this function with one argument?
In the end, I didn't need it. But since make_error accepts variable args, I
figured this was more flexible. I can simplify it in the end, I only pass a
single param.
================
Comment at: lib/Format/Format.cpp:1890
}
- FormatStyle Style = getLLVMStyle();
- Style.Language = getLanguageByFileName(FileName);
+ FormatStyle::LanguageKind Language = getLanguageByFileName(FileName);
----------------
ioeric wrote:
> 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.
The thing is, it made a lot of sense to have a default constructed FormatStyle
before when we were sure to always return it. Since now we only return a Style
under 2 specific conditions (found a config file, or did not return the
FallbackStyle), otherwise return an error, I feel like it's more clear this
way. But if you firmly disagree, I can go back.
To make returning the Style with current Language more clear, I could use a
function that ties the two together and returns it (perhaps a local lambda),
like StyleWithLanguage(Style, Language).
================
Comment at: lib/Format/Format.cpp:1900
+ FormatStyle FallbackStyle = getNoStyle();
+ if (!getPredefinedStyle(FallbackStyleName, Language, &FallbackStyle)) {
+ return make_string_error("Invalid fallback style \"" +
----------------
ioeric wrote:
> Nit: prefer no curly brace around one liners. Same below.
Will do.
================
Comment at: lib/Format/Format.cpp:1901
+ if (!getPredefinedStyle(FallbackStyleName, Language, &FallbackStyle)) {
+ return make_string_error("Invalid fallback style \"" +
+ FallbackStyleName.str());
----------------
ioeric wrote:
> 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).
I agree with you, returning more specific error codes would be useful. However,
I'm thinking we go at it incrementally. The point of this change is to have
getStyle return when an error occurs so that we return non-zero from main, and
more importantly, not use the fallback style when this happens.
In that respect, what I'm wondering is whether I should just leave the errs()
output in getStyle, and simply return an error code, or keep what I've done
(returning StringError) as a stepping stone in the right direction - that is,
eventually returning customized format-relayed error codes, as you say.
================
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) {
----------------
ioeric wrote:
> Is this indentation intended?
No. I will definitely do a formatting pass. I'm using Visual Studio with the
clang-format extension, which works alright, but Visual Studio fights against
it with its own formatting. Indeed, the reason I even started contributing to
clang-format was because I wanted to improve the VS extension. I've already
pushed a few changes to it, and intend to do a little more to make the
experience simpler. In any case, my next diff upload will have proper
formatting everywhere.
https://reviews.llvm.org/D28081
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
