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

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 27 12:01:35 PST 2016


ioeric added inline comments.


================
Comment at: include/clang/Format/Format.h:862
 ///
 /// \returns FormatStyle as specified by ``StyleName``. If no style could be
 /// determined, the default is LLVM Style (see ``getLLVMStyle()``).
----------------
Is this still true?


================
Comment at: lib/Format/Format.cpp:424
 
+template <typename... ArgTs> llvm::Error make_string_error(ArgTs &&... Args) {
+  return llvm::make_error<llvm::StringError>(std::forward<ArgTs>(Args)...,
----------------
Why do you need a template if you only use this function with one argument?


================
Comment at: lib/Format/Format.cpp:1890
   }
-  FormatStyle Style = getLLVMStyle();
-  Style.Language = getLanguageByFileName(FileName);
+  FormatStyle::LanguageKind Language = getLanguageByFileName(FileName);
 
----------------
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.


================
Comment at: lib/Format/Format.cpp:1900
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Language, &FallbackStyle)) {
+    return make_string_error("Invalid fallback style \"" +
----------------
Nit: prefer no curly brace around one liners. Same below.


================
Comment at: lib/Format/Format.cpp:1901
+  if (!getPredefinedStyle(FallbackStyleName, Language, &FallbackStyle)) {
+    return make_string_error("Invalid fallback style \"" +
+                             FallbackStyleName.str());
----------------
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). 


================
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");
+	if (!FormatStyleOrError) {
----------------
Is this indentation intended? 


================
Comment at: lib/Tooling/Refactoring.cpp:87
+	llvm::Expected<format::FormatStyle> FormatStyleOrError = format::getStyle(Style, FilePath, "LLVM");
+	if (!FormatStyleOrError) {
+		llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
----------------
amaiorano wrote:
> As I wrote above, by returning the string, the caller has the burden of outputting the error. More importantly, getStyle may already have written other errors to errs(), so I'm not sure this makes sense.
> 
> If we go with only returning error code, I suppose I'd use llvm::ErrorOr<FormatStyle> as the return type.
I don't think `getStyle` should write to `errs()` if it returns an `Error`. 


https://reviews.llvm.org/D28081





More information about the cfe-commits mailing list