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

Antonio Maiorano via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Dec 27 12:38:00 PST 2016


amaiorano added a comment.

Thank you, @ioeric for your feedback!



================
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()``).
----------------
ioeric wrote:
> Is this still true?
No, it's not true, and I intend to update this comment. Thanks for pointing it out :)


================
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)...,
----------------
ioeric wrote:
> Why do you need a template if you only use this function with one argument?
In the end, I didn't need it. But since make_error accepts variable args, I figured this was more flexible. I can simplify it in the end, I only pass a single param.


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


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


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



================
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) {
----------------
ioeric wrote:
> Is this indentation intended? 
No. I will definitely do a formatting pass. I'm using Visual Studio with the clang-format extension, which works alright, but Visual Studio fights against it with its own formatting. Indeed, the reason I even started contributing to clang-format was because I wanted to improve the VS extension. I've already pushed a few changes to it, and intend to do a little more to make the experience simpler. In any case, my next diff upload will have proper formatting everywhere.


https://reviews.llvm.org/D28081





More information about the cfe-commits mailing list