[PATCH] D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 28 00:57:39 PST 2016


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





More information about the cfe-commits mailing list