[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D27440#624917, @djasper wrote:

> Yes.. return non-zero seems right. This is an error condition.


Hi @djasper ,

I started looking into making the changes to clang-format to have it return an error code when it's unable to parse the .clang-format file, but I'm not quite sure what the best approach is. The function that needs modifying is getStyle <https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L1879>. There are multiple places in there where it outputs to stderr (llvm::errs()) when something goes wrong, like here <https://github.com/llvm-mirror/clang/blob/master/lib/Format/Format.cpp#L1904-L1905>.

So here's what I'm thinking in terms of solutions when an error occurs in getStyle:

1. Throw an exception. This is unidiomatic in clang-format, and isn't something I'm particularly fond of, but it means not modifying the return type, and not having to modify the tests since they assume the green path.

2. Return an llvm::ErrorOr. Of course, this changes the signature, which means changing the few places that call getStyle. From what I can tell, it's only being called in a couple of places, and in the tests, so perhaps it's not terrible.

3. Return an llvm::Optional. This is similar to ErrorOr, except that it may allow us to codify the fallback behaviour on the outside of this call. What I mean is that with Optional, we wouldn't have to pass in the fallback style, but rather, the function could return an error when the input style isn't found or parsed correctly, etc., and the calling code can decide what to do with the error: either stop right there (return an error code from main), or it can try to get the fallback style. Something like:



  // The case where we don't care about errors and want to use a fallback style:
  FormatStyle fallbackStyle = getLLVMStyle();
  FormatStyle formatStyle = getStyle("", fileName).getValueOr(fallbackStyle);
  
  
  // The case where we do care about errors
  auto maybeFormatStyle = getStyle("", fileName);
  if (!maybeFormatStyle)
      return 0;
  
  FormatStyle formatStyle = maybeFormatStyle.getValue();

Obviously this third option would change the most code, but maybe it makes more sense for getStyle to not have this notion of fallback style within it, as it effectively hides errors.

Would love to hear your thoughts.


https://reviews.llvm.org/D27440