ioeric added a comment. In https://reviews.llvm.org/D28081#633103, @amaiorano wrote:
> 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. This is a good YAQ, which IMO should be tackled in a separate patch. In this patch though, it might be easier to proceed by keeping the original behavior and leaving a `FIXME`. In general, reviewers like smaller patches with single purpose :) https://reviews.llvm.org/D28081 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
