ioeric added inline comments.
================
Comment at: lib/Format/Format.cpp:1890
}
- FormatStyle Style = getLLVMStyle();
- Style.Language = getLanguageByFileName(FileName);
+ FormatStyle::LanguageKind Language = getLanguageByFileName(FileName);
----------------
amaiorano wrote:
> 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).
I'd go with the old way with some comments, which should be clear enough.
================
Comment at: lib/Format/Format.cpp:1901
+ if (!getPredefinedStyle(FallbackStyleName, Language, &FallbackStyle)) {
+ return make_string_error("Invalid fallback style \"" +
+ FallbackStyleName.str());
----------------
amaiorano wrote:
> 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.
>
I think the interface should no longer write to `errs()` if we want to
introduce error handling (either with error codes or `llvm::Error`).
`StringError` does not really collide with error codes AFAICT - it can also
carry error codes which you would have return directly. And even if you just
return error codes, you would need some sort of customized error codes right?
https://reviews.llvm.org/D28081
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
