MyDeveloperDay added a comment. I have no concerns with this patch other than the consideration of the defaults.
My concern is whatever our choice we will generate churn on existing code where the .clang-format file has AfterExternBlock = true (or its true in inbuilt BasedOnStyle type) https://github.com/search?q=%22AfterExternBlock%3A+true%22&type=Code This is also true for all Mozilla and Allman styles, I'd be happy to give this a LGTM, but I feel like I'll end up saying "told you so on the choice of defaults" when the complaints come in, unless we are able to say `Style.IndentExternBlock = Style.BraceWrapping.AfterExternBlock` The problem is really that we need to detect the existence of IndentExternBlock missing from the config and a boolean isn't the best choice for this because we have 3 states (NotSpecified,Indent,DontIndent) I'm uncomfortable with the decision we've made that says IndentExternBlock is by default false when previously if we were putting a break we were following the normal indenting rules of all other control blocks parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/Style.IndentExternBlock); If we can't solve that, I'm not sure I'm happy to Accept, but code wise I'm fine with everything else. I'm not sure if I'm overly worrying. There is some code in the `void mapping(IO &IO, FormatStyle &Style)` that could show you a way to do this.. but I'm also constantly surprised by how many of the enumeration cases started out as booleans only later to have to be converted to enums. The more I think about this the more I think the problem can probably be dealt with better by making it an enumeration. (even if you support true and false to mean "indent" and "don't indent" template <> struct ScalarEnumerationTraits<FormatStyle::BracketAlignmentStyle> { static void enumeration(IO &IO, FormatStyle::BracketAlignmentStyle &Value) { IO.enumCase(Value, "Align", FormatStyle::BAS_Align); IO.enumCase(Value, "DontAlign", FormatStyle::BAS_DontAlign); IO.enumCase(Value, "AlwaysBreak", FormatStyle::BAS_AlwaysBreak); // For backward compatibility. IO.enumCase(Value, "true", FormatStyle::BAS_Align); IO.enumCase(Value, "false", FormatStyle::BAS_DontAlign); } }; CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75791/new/ https://reviews.llvm.org/D75791 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
