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

Antonio Maiorano via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Dec 31 08:35:16 PST 2016


amaiorano added a comment.

Hello everyone, so after a few more tests, I've uncovered a bug and perhaps a different meaning for fallback style. First, the bug: if you set fallback style to "none", clang-format will perform no replacements. This happens because getStyle will first initialize its local Style variable to LLVM style, and then because a fallback style is set, will then set it to the "none" style, will ends up setting Style.DisableFormatting to true. After that, when we parse YAML (either from Style arg or a config file), we use the Style variable as the "template" for fields that haven't been set. In this case, the "none" fallback style causes DisableFormatting to remain true, so no formatting will take place.

As it happens, my first diff patch uploaded here fixed this issue by accident. Instead of reusing the same local Style variable, I declared one for each case where we'd need to parse. The fallback style case would use its own variable, FallbackStyle, which would not be used as the template style when parsing the YAML config.

What's interesting is that the way the code is originally written allows you to use fallback style as a way to set the "base" configuration for which the subsequently parsed YAML overlays. For example, if I don't set fallback style, the assumed base style is "LLVM", and any YAML parsed modifies this LLVM base style. But if I pass a fallback style of "Mozilla", then this becomes the base style over which the YAML overlays.

So to my mind, we have 2 approaches to fix the "none" style bug:

1. Go with a similar approach to what I did originally; that is, we always assume LLVM as the base style, and make sure that the fallback style is not used as the base style, but rather only as the style to return if none is found. I think this is what FallbackStyle was originally intended for.

2. Allow fallback style to maintain its current meaning - that is, as a way to set the base style when "style" is "file" or YAML. In this case, I believe the right thing is to treat FallbackStyle set to "none" as though no fallback style were passed in at all. Concretely, we might want t to modify getPredefinedStyle to return LLVM style when "none" is passed in, instead of what it does now. I personally think this is more confusing, and also introduces more risk.

Let me know what you think. If we go with option 1, I could fold the fix into this change.



================
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;
-  }
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, &FallbackStyle))
----------------
Going over my diff, I realize that I didn't revert this local FormatStyle instance to make use of the top-level declared "FormatStyle Style" local variable. I will revert this one too.


https://reviews.llvm.org/D28081





More information about the cfe-commits mailing list