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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

The patch LGTM now. I'll accept both this and the one for clang-tool-extra when it is ready.

Regarding builbots, we have bots that continually run builds/tests (http://lab.llvm.org:8011/). Many buildbots test llvm and clang as well as clang-tools-extra (e.g. with `ninja check-all`) at some revision. Also note that although llvm/clang and clang-tools-extra are different repos, they do share the same revision sequence. So if clang-tools-extra is in a inconsistent state, many buildbots can fail and affect llvm/clang builds. Unfortunately, there is no atomic way to commit two revisions to two repositories, so we just commit them quickly one after another so that we do less damage. Do you have commit access to LLVM btw?



================
Comment at: lib/Format/Format.cpp:1900
 
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, &Style)) {
-    llvm::errs() << "Invalid fallback style \"" << FallbackStyle
-                 << "\" using LLVM style\n";
-    return Style;
-  }
+  // FIXME: If FallbackStyle is explicitly "none", replacements are disabled.
+  if (!getPredefinedStyle(FallbackStyle, Style.Language, &Style))
----------------
maybe "format is disabled" is clearer? 


================
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");
----------------
amaiorano wrote:
> ioeric wrote:
> > There is a `NewReplacements` below which is also `llvm::Expected<T>`. `FormatStyleOrError` is  a fine name, but to be we have been naming `Expected` types without `OrError` postfix, so I'd go without `OrError` postfix for consistency append `OrError` postfix to other expected variables. Personally, I find expected variables without `OrError` postfix easier to understand, especially in error checking. For example, `if (!FormatStyleOrError)` is a bit awkward to read while `if (!FormatStyle)` is more straight-forward IMO.
> > 
> > Same for other changes.
> Agreed. For consistency, would you prefer I also use 'auto' for the return type rather than llvm::Expected<format::FormatStyle> as is done for NewReplacements?
`auto` is fine.


================
Comment at: unittests/Format/FormatTestObjC.cpp:72
 TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
                                           "- (id)init;");
----------------
amaiorano wrote:
> amaiorano wrote:
> > ioeric wrote:
> > > amaiorano wrote:
> > > > In these tests, I'm assuming getStyle returns a valid FormatSyle. I could add the same types of validation as in the FormatStyle.GetStyleOfFile tests.
> > > Please add proper checking as above for returned values.
> > Hmm, so I could replace the Style member of the fixture class with Expected<FormatStyle>, and then change all "Style." with "Style->" in the rest of the test file, or only in this specific test, I could store the result in a local Expected<FormatStyle>, check that it's valid, and then assign to Style. The latter is simpler; only question I have is how to name the local variable - can I go with StyleOrError? Style2?
> Another option here is to make this a non-fixture TEST and just declare a local Expected<FormatStyle> Style for this specific test, which would work fine.
Yeah, making this non-fixture sounds like a better idea.  `StyleOrError` is fine here.


https://reviews.llvm.org/D28081