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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Some nits. Some is almost good :)

BTW, do you have clang-tools-extra in your source tree? There are also some references in the subtree to the changed interface. It would be nice if you could also fix them in a separate patch and commit these two patches together (I mean, within a short period of time) so that you wouldn't break build bots.

References should be found in these files:

  extra/change-namespace/ChangeNamespace.cpp
  extra/clang-move/ClangMove.cpp
  extra/include-fixer/tool/ClangIncludeFixer.cpp	 
  extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  extra/clang-tidy/ClangTidy.cpp

Thanks!



================
Comment at: lib/Format/Format.cpp:424
 
+llvm::Error make_string_error(const llvm::Twine &Message) {
+  return llvm::make_error<llvm::StringError>(Message,
----------------
Maybe make this `inline`?


================
Comment at: lib/Format/Format.cpp:1901
+  // FIXME: If FallbackStyle is explicitly "none", this effectively disables
+  // replacements. Fix this by setting a separate FormatStyle variable and
+  // returning it when we mean to return the fallback style explicitly.
----------------
I'd state the problem with a specific solution only if I am sure it is the best solution.


================
Comment at: lib/Format/Format.cpp:1955
           FS->getBufferForFile(ConfigFile.str());
       if (std::error_code EC = Text.getError()) {
+        return make_string_error(EC.message());
----------------
Redundant braces. Same below.


================
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");
----------------
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.


================
Comment at: unittests/Format/FormatTest.cpp:10972
+  auto ErrorMsg4 = llvm::toString(Style4.takeError());
+  ASSERT_GT(ErrorMsg4.length(), 0);
+
----------------
This is a bit strange... Just `llvm::consumeError(Style.takeError())` if you don't bother to check the error message, e.g. with `llvm::StringRef::starswith` or RE match.


================
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:
> 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.


https://reviews.llvm.org/D28081