Report clang-format issues

[StephanTLavavej]

We have a lot of workarounds for clang-format misbehavior. If anyone has free time, it would be very helpful to identify the top issues causing problems, prepare reduced test cases, and report them upstream. (Or fix them, if anyone wants to become a clang-format contributor.) Some issues may already be known: https://github.com/llvm/llvm-project/issues?q=is%3Aissue+is%3Aopen+label%3Aclang-format

We often use // clang-format off so that can be a good way to find examples - try removing it and see if something egregious happens. (However, not every occurrence of // clang-format off is working around misbehavior. Sometimes clang-format would do a reasonable job, but we have a particular cosmetic preference, or need to match the formatting of external code. xcharconv_ryu.h is an example of "match external code".) Another common workaround is an empty comment // at the end of a line to force line wrapping.

A non-exhaustive list of common problems:

• clang-format sometimes handles concepts poorly, particularly requires expressions (and we have particular preferences for requires stuff versus requires(stuff) versus requires (stuff) depending on the code).
  • Especially see #2064: clang-format 12 has extreme trouble with complex non-parenthesized requires-clauses, even within // clang-format off - it gets confused about the level of indentation and damages the rest of the file. We need to add parentheses in order to avoid this.
• Variable templates are commonly mishandled, and we love variable templates so this affects us everywhere.
• In certain situations, clang-format doesn't respect our 120-column limit.
• The documentation at https://releases.llvm.org/13.0.0/tools/clang/docs/ClangFormatStyleOptions.html contains numerous typos, some of which significantly affect usability. (e.g. SpacesInLineCommentPrefix is extremely damaged)

We love clang-format :heart_eyes_cat: which is why we've formatted the entire STL since VS 2019 16.0 and have a validator that breaks the build when a file isn't clang-formatted. We just want it to be even better!

[H-G-Hristov]

What about the ability to control the bin packing of arguments and parameters of functions and templates similar to what I see in some Microsoft APIs something like:

template <
      typename T1,
      typename T2,
      typename T3,
>
auto func (
      T1 a1,
      T2 a2,
      T3 a3
)
.....

For me it makes sense.

[StephanTLavavej]

@H-G-Hristov Thanks for the suggestion - that could be interesting for many projects, but MSVC's STL has not used that style. This issue is tracking how clang-format has trouble with the STL's current style configuration, not a general collection of feature requests.

[BrandonPacewic]

Are there any particular examples of where the 120 column limit fails or does not behave as expected?

[StephanTLavavej]

@BrandonPacewic Sure, here's one: https://github.com/microsoft/STL/blob/1a20fe1133d711a647bbb135d98743f91b7be323/stl/inc/variant#L1017-L1027 We've enforced line wrapping with empty comments. If we remove those comments, clang-format 13.0.1 emits:

    template <class _Ty,
        enable_if_t<
            sizeof...(_Types) != 0
                && !is_same_v<_Remove_cvref_t<_Ty>,
                    variant> && !_Is_specialization_v<_Remove_cvref_t<_Ty>, in_place_type_t> && !_Is_in_place_index_specialization<_Remove_cvref_t<_Ty>> && is_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>,
            int> = 0>
    constexpr variant(_Ty&& _Obj) noexcept(is_nothrow_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>)
        : _Mybase(in_place_index<_Variant_init_index<_Ty, _Types...>::value>, static_cast<_Ty&&>(_Obj)) {
        // initialize to the type selected by passing _Obj to the overload set f(Types)...
    }

with 216 columns vastly exceeding our 120 limit: https://github.com/microsoft/STL/blob/1a20fe1133d711a647bbb135d98743f91b7be323/.clang-format#L78

[BrandonPacewic]

@StephanTLavavej Is there any particular reason to use clang-format 13.0.1 over 15.0.0? When switching to the newest version it seems to work as expected.

    template <class _Ty, enable_if_t<sizeof...(_Types) != 0 && !is_same_v<_Remove_cvref_t<_Ty>, variant>
                                         && !_Is_specialization_v<_Remove_cvref_t<_Ty>, in_place_type_t>
                                         && !_Is_in_place_index_specialization<_Remove_cvref_t<_Ty>>
                                         && is_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>,
                             int> = 0>
    constexpr variant(_Ty&& _Obj) noexcept(is_nothrow_constructible_v<_Variant_init_type<_Ty, _Types...>, _Ty>)
        : _Mybase(in_place_index<_Variant_init_index<_Ty, _Types...>::value>, static_cast<_Ty&&>(_Obj)) {
        // initialize to the type selected by passing _Obj to the overload set f(Types)...
    }

Switching to the newer version slightly changes how the .clang-format will be interpreted.

Before:

    _Variant_destroy_layer_()                               = default;
    _Variant_destroy_layer_(const _Variant_destroy_layer_&) = default;
    _Variant_destroy_layer_(_Variant_destroy_layer_&&)      = default;
    _Variant_destroy_layer_& operator=(const _Variant_destroy_layer_&) = default;
    _Variant_destroy_layer_& operator=(_Variant_destroy_layer_&&) = default;

After:

    _Variant_destroy_layer_()                                          = default;
    _Variant_destroy_layer_(const _Variant_destroy_layer_&)            = default;
    _Variant_destroy_layer_(_Variant_destroy_layer_&&)                 = default;
    _Variant_destroy_layer_& operator=(const _Variant_destroy_layer_&) = default;
    _Variant_destroy_layer_& operator=(_Variant_destroy_layer_&&)      = default;

It might take some tweaking to get the config file back to where you want it but switching to the newer version seems to fix problems like this. Unless of course, if the format checker would then break the build because it is expecting an older version of clang-format. Depending on what you use to check the format.

[BrandonPacewic]

I now know why the STL uses clang-format version 13.0.1 over 15.0.0.

[StephanTLavavej]

Thanks, that's really good news! As you mentioned, we use the clang-format version that ships in VS, which is regularly updated to the latest release version (with a time delay depending on when the Clang and VS releases happen). So we'll pick up clang-format 14 and then 15 in the future. At that time we can see what issues (if any) remain.

[StephanTLavavej]

clang-format 15 is indeed much better, but it still has trouble with some things like concept definitions that begin with !.

[CaseyCarter]

It would be very nice if RequiresClausePosition were split into LeadingRequiresClausePosition and TrailingRequiresClausePosition.

[StephanTLavavej]

clang-format 18 is greatly improved (especially for concepts and variable templates), but it now has a frustrating quirk where its desire to align = in aliases can cause it to exceed the 120 column limit, as if it's performing the alignment as the last step.