Page MenuHomePhabricator

[clang-format] Improve clang-formats handling of concepts
Needs RevisionPublic

Authored by MyDeveloperDay on May 12 2020, 3:06 AM.

Details

Summary

This is a starting point to improve the handling of concepts in clang-format. There is currently no real formatting of concepts and this can lead to some odd formatting, e.g.

requires(R range) {
  typename Iterator_type<R>;
  { begin(range) }
  ->Iterator_type<R>;
  { end(range) }
  ->Iterator_type<R>;
  requires Input_iterator<Iterator_type<R>>();
};

template <typename T> concept Large = sizeof(T) > 10;

template <typename T, typename U> concept FooableWith = requires(T t, U u) {
  typename T::foo_type;
  { t.foo(u) }
  ->typename T::foo_type;
  t++;
};

The revision starts by resolving the additional newline added before the implicit conversion constraint and ensures that the concept keyword is always on the line below template<>

template <typename T>
concept bool EqualityComparable = requires(T a, T b) {
  { a == b } -> bool;
};

Additional Options include the following to allow a 1 level indentation of the requires clauses (as seems to be popular in a couple of sources, online reference and conference slides about concepts)

  • IndentRequires (indent the "requires keyword after template")

e.g.

template <class I, class S>
   requires Sentinel<S, I>()
constexpr void advance(I &i, S bound) noexcept(noexcept(++i != bound)) {
  while (i != bound) {
    ++i;
  }
}

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MyDeveloperDay added a reviewer: saar.raz.
MyDeveloperDay added a subscriber: saar.raz.

Add additional parsing of concept and require keywords to allow formatting

Include some more of @saar.raz examples in the tests

MyDeveloperDay edited the summary of this revision. (Show Details)May 12 2020, 12:35 PM
MyDeveloperDay marked an inline comment as done.May 12 2020, 12:38 PM
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1952

This change it to resolve the missing gap between && and Concept2, clang-format incorrectly labels the && as TT_PointerOrReference

template <class I> void f(I i) requires Concept1<I> &&Concept2<i> {
  // ...
}

vs

template <class I> void f(I i) requires Concept1<I> && Concept2<i> {
  // ...
}

Add support for IndentRequires add additional tests for interaction with SpaceBeforeParens

MyDeveloperDay edited the summary of this revision. (Show Details)May 12 2020, 2:37 PM
MyDeveloperDay updated this revision to Diff 263655.EditedMay 13 2020, 3:09 AM
MyDeveloperDay edited the summary of this revision. (Show Details)

Introduce the concept of TT_ConstraintJuncitons for both && and || (Conjunctions and Disjunctions) as these were confusing the layout as often they are treated as TT_BinaryOperator and or TT_PointerOrReference

This allow more specific concepts base rules and keeps the code separate from other TT_BinaryOperator rules

miscco added a subscriber: miscco.May 19 2020, 2:31 AM

Awesome, Thank you very much, I dragged my feet on starting to implement something for real.

As a high level comment I think we need to handle requires expressions to get this correct, as requires requires would otherwise to bad things.

From my early brainstorming i got the following options I thought useful. This is in no ways a request for you to implement them, but I would want to get them out here in case they are usefull

/// Different styles for merging short requires-clauses with the template
  /// decalaration
  enum ShortRequiresClauseStyle {
    /// Never merge requires clauses into a single line.
    /// \code
    ///   template <class T>
    ///   requires someConstraint<T>
    ///   T foo();
    /// \endcode
    SRCS_Never,
    /// Only merge requires clauses with a single constraint.
    /// \code
    ///   template <class T> requires someConstraint<T>
    ///   T foo();
    ///
    ///   template <class T>
    ///   requires someConstraint<T> && otherConstraint<T>
    ///   T bar();
    /// \endcode
    SRCS_Single,
    /// Merge requires clauses if they are short
    /// \code
    ///   template <class T> requires short<T> && shorter<T>
    ///   T foo();
    ///
    ///   template <class T>
    ///   requires someReallyLongElaborateConstraintThatIsReallyLong<T>
    ///   T bar();
    ///
    ///   template <class T>
    ///   requires someLongConstraint<T> && otherLongConstraint<T>
    ///   T baz();
    /// \endcode
    SRCS_Short,
    /// Always try to merge the requires clause with the template declaration
    /// \code
    ///   template <class T> requires someLongConstraint<T> &&
    ///                               otherLongConstraint<T>
    ///   T foo();
    /// \endcode
    SRCS_Always,
  };

  /// Dependent on the value, requires-clauses can be put on the same line
  /// as the template declaration.
  ShortRequiresClauseStyle AllowShortRequiresClause;

  /// Dependent on the value, requires-expressions can be a single line
  /// Always try to merge the requires clause with the template declaration
  /// \code
  ///   template <class T> requires someLongConstraint<T> &&
  ///                               requires { T{}; }
  ///   T foo();
  /// \endcode
  bool AllowShortRequiresExpression;

   /// Dependent on the value break before or after constraint expressions.
  enum ConstraintExpressionBreakingStyle {
    /// Break after constraint expressions but multiple may be on single line
    /// \code
    ///   template <class T> requires someLongConstraint<T> &&
    ///                               otherLongConstraint<T>
    ///   T foo();
    ///
    ///   template <class T> requires short<T> && shorter<T> &&
    ///                               otherLongConstraint<T>
    ///   T bar();
    /// \endcode
    CEBS_After,
    /// Break after constraint expressions.
    /// \code
    ///   template <class T> requires short<T> &&
    ///                               shorter<T> &&
    ///                               otherLongConstraint<T>
    ///   T bar();
    /// \endcode
    CEBS_AfterSingleExpression,
    /// Break before constraint expressions but multiple may be on single line
    /// \code
    ///   template <class T> requires someLongConstraint<T> &&
    ///                               otherLongConstraint<T>
    ///   T foo();
    ///
    ///   template <class T> requires short<T> && shorter<T>
    ///                            && otherLongConstraint<T>
    ///   T bar();
    /// \endcode
    CEBS_Before,
    /// Break before constraint expressions.
    /// \code
    ///   template <class T> requires short<T>
    ///                            && shorter<T>
    ///                            && otherLongConstraint<T>
    ///   T foo();
    /// \endcode
    CEBS_BeforeSingleExpression,
  };

  /// The constraint expressions style to use.
  ConstraintExpressionBreakingStyle BreakBeforeConstraintExpression;

  /// Dependent on the value wrap before or after requires expressions.
  enum BraceWrappingRequiresExpressionStyle {
    /// Do not wrap braces of requires expressions
    /// \code
    ///   template <class T> requires requires {  T{};
    ///                                           T(int); }
    ///   T foo();
    /// \endcode
    BWARES_NoWrap,
    /// Wrap after requires expressions.
    /// \code
    ///   template <class T> requires requires { T{};
    ///                                          T(int);
    ///                                        }
    ///   T foo();
    /// \endcode
    BWARES_WrapAfter,
    /// Wrap after requires expressions.
    /// \code
    ///   template <class T> requires requires
    ///                               { T{};
    ///                                 T(int); }
    ///   T foo();
    /// \endcode
    BWARES_WrapBefore,
    /// Wrap after requires expressions with a new line.
    /// \code
    ///   template <class T> requires requires
    ///                               {
    ///                                 T{};
    ///                                 T(int); }
    ///   T foo();
    /// \endcode
    BWARES_WrapBeforeWithNewline,
    /// Wrap before requires expressions.
    /// \code
    ///   template <class T> requires requires
    ///                               { T{};
    ///                                 T(int);
    ///                               }
    ///   T foo();
    /// \endcode
    BWARES_WrapBoth,
    /// Wrap before requires expressions.
    /// \code
    ///   template <class T> requires requires
    ///                               {
    ///                                 T{};
    ///                                 T(int);
    ///                               }
    ///   T foo();
    /// \endcode
    BWARES_WrapBothWithNewline,
  };

  /// The requires expressions style to use.
  BraceWrappingRequiresExpressionStyle BraceWrappingRequiresExpression;

My problem was that I had no idea how to properly tokenize the logical-or-constraint-expression. I have pushed my super clumsy WIP stuff here:
https://github.com/miscco/llvm-project/pull/new/clang_format_concepts

If I can help you in any way please tell me

clang/lib/Format/TokenAnnotator.cpp
1595–1599

Should that really be tok::arrow and not tok::kw_requires?

3519

I think we should have an option to allow short requires clauses on the same line similar to allow short functions

clang/lib/Format/UnwrappedLineParser.cpp
2308

I believe we should separate between constraint expressions aka: tempalte< > requires Concept1 && Concept2 and requires expressions aka requires { ... }

The two are quite different. That said the handling of requires expressions should be "simple" as we only need to find the matching "}".

As an upside this would also handle the famous requires requires

MyDeveloperDay marked 6 inline comments as done.May 19 2020, 3:02 AM
MyDeveloperDay added a subscriber: STL_MSFT.
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1595–1599

This is to prevent the breaking between } and -> in the following (which could be some way from the requires

{ t.foo(u) } -> typename T::foo_type;

3519

Actually at present it will pull short requires onto a new line, this particular line is about keeping concept on a new line (most examplesI've seen are like this..i.e.

template<typename T>
concept...

rather than

template<typename T> concept...

For me the natural thing here is to enforce a newline and then relax that later if we think it needs it, but I'm wary of adding too many options upfront.

This is virgin territory as most style guides out there don't specify anything for concepts yet, to be honest i'd prefer to establish an LLVM style and wait for the other style guides to catch up. (I know we will revisit this later), just want to keep the initial commit small. (incremental development is encouraged in LLVM) and multiple options will be a huge commit that never gets passed.

clang/lib/Format/UnwrappedLineParser.cpp
2308

This is effectively the if at line 2305 seeing "requires {" or requires(Foo t) {" sets off down the parseBlock path which can itself include requires.

If you do have an example you think would break this please feel free to add that in a comment and I'll add it to the unit tests and rework accordingly.

My exposure to concepts is still pretty new, I'm not even sure I've covered all the places it can be used, but I wanted to start because at present I see alot of the MS STL(@STL_MSFT ) covered with // clang-format off around the concepts.

miscco added inline comments.May 19 2020, 3:17 AM
clang/lib/Format/TokenAnnotator.cpp
3529

I think that your change should actually come in here where we determine what to do after a ClosesTemplateDeclaration

With an potential new option AlwaysBreakConceptDeclarations that should probably default to AlwaysBreakTemplateDeclarations this would read

if (Right.Previous->ClosesTemplateDeclaration &&
    Right.Previous->MatchingParen &&
    Right.Previous->MatchingParen->NestingLevel == 0) { 
    if (Right.is(tok::kw_requires)) {
      switch(Style.AllowShortRequiresClause) {
        case FormatStyle::SRCS_Never: 
          return true;
        case FormatStyle::SRCS_Always: 
          return false;
        case FormatStyle::SRCS_Single:
          // TODO: Determine whether there is a single constraint 
          return true;
        case FormatStyle::SRCS_Short: 
          // TODO: Determine whether the constraint clause is short enough
          return true;
      } 
    } else if (Right.is(tok::kw_concept)) {
      return Style.AlwaysBreakConceptDeclarations == FormatStyle::BTCS_Yes);
    } else {
      return Style.AlwaysBreakTemplateDeclarations == FormatStyle::BTDS_Yes);
    }
}
MyDeveloperDay planned changes to this revision.May 19 2020, 3:39 AM
MyDeveloperDay marked 4 inline comments as done.
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
3529

yes this is a better place

Move break before concept into correct clause
Add requires requires test

You are missing to check the boolean in CHECK_PARSE_BOOL in FormatTest.cpp

clang/lib/Format/TokenAnnotator.cpp
1595–1599

I believe this should carry some state that we are inside of an requires-expression.

3519

Personally I agree that this is the natural style, at the same time there are configurations that allow short template declarations on a single line, which I believe would be similarly welcome.

That said I agree adding this later is fine

clang/lib/Format/UnwrappedLineParser.cpp
2308

Yes that was also the reason I tried it out because I often added those // clang-format off snippets once too often

miscco added inline comments.May 19 2020, 5:10 AM
clang/lib/Format/UnwrappedLineParser.cpp
2305

I believe this should be parseConstraintExpression because that is the term of art in the standard.

The requires expression is what is the requieres { } and that can be part of an constraint expression

miscco added inline comments.May 19 2020, 6:05 AM
clang/lib/Format/UnwrappedLineParser.cpp
2336

I guess you could use parseBracedList(/*ContinueOnSemicolons=*/false, /*IsEnum=*/false,/*ClosingBraceKind=*/tok::greater); here?

miscco added inline comments.May 19 2020, 10:45 PM
clang/lib/Format/UnwrappedLineParser.cpp
2336

To be more specific, I am concerned what happens if you have multiple template arguments where a linebreak should occur. Is this still happening here?

template <typename someLongTypename1, typename someLongTypename2>
concept someVeryLongConceptName = someVeryLongConstraintName1<someLongTypename1 && someLongTypename2>;
clang/unittests/Format/FormatTest.cpp
16656

I think concept bool should just be concept occurs below

MyDeveloperDay planned changes to this revision.May 20 2020, 2:29 AM
MyDeveloperDay marked 3 inline comments as done.
MyDeveloperDay added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1595–1599

I'm not sure how possible that is as its dealt with in terms of normal parseBlock() handling so there might not be a possiblity to label the -> (which is actually what I'm just doing here) the other rules regarding TT_TrailingReturnArrow will ensure its doesn't get broken and there is spaces around it.

MyDeveloperDay marked 2 inline comments as done.

Add new AlwaysBreakBeforeConceptDeclarations

Better handling of requires expressions vs constraint expression

add more concept tests from the twitter-sphere

MyDeveloperDay marked 7 inline comments as done.May 20 2020, 5:54 AM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2305

I'm making some of these changes, the problem is with the form

concept id = Foo<T> && Bar<T>

in this case the

Foo<T> && Bar<T>

will be processed by the parseStructualElement() and this gets confused with the && being a TT_PointerOrRefernce and not a TT_BinaryOperator and that throws everything out.

I think parseStructualElement() is thinking Foo<T> && is part of

foo(Foo<T> &&abc);

and not

Foo<T> && abc<T>

We need to know we are in a concept expression

2336

This is formatted as

template <typename someLongTypename1, typename someLongTypename2>
concept someVeryLongConceptName =
    someVeryLongConstraintName1<someLongTypename1 && someLongTypename2>;

Question: Should I add my wip work as a child revision or what would you suggest

MyDeveloperDay marked 2 inline comments as done.May 20 2020, 7:35 AM

Question: Should I add my wip work as a child revision or what would you suggest

Certainly your revision on github was useful for me to take a look at, but to be honest this change is getting large and I don't like landing such a large change in one go but rather make small incremental improvements (I believe this is the LLVM way)

Feel free to create a revision (and reference it here). Once we land some form of initial implementation I feel we can start making improvement as we detect corner cases or adding new options (and that might be more easily spread between developers)

To be honest I would rather start getting bugs from external users than clang-format being the butt of peoples complaints https://twitter.com/the_moisrex/status/1262982971973423105

It seems the spacing of the binary operator is not yet stable. This test is breaking for me:

verifyFormat("template <typename T>\nconcept someConcept = Constraint1<T> && Constraint2<T>;");

It seems the spacing of the binary operator is not yet stable. This test is breaking for me:

verifyFormat("template <typename T>\nconcept someConcept = Constraint1<T> && Constraint2<T>;");

Hmm... are you upto date

$ ./FormatTests.exe --gtest_filter=*Concept*
Note: Google Test filter = *Concept*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.ConceptsAndRequires
[       OK ] FormatTest.ConceptsAndRequires (642 ms)
[----------] 1 test from FormatTest (645 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (650 ms total)
[  PASSED  ] 1 test.
miscco added inline comments.May 23 2020, 3:54 AM
clang/lib/Format/UnwrappedLineParser.cpp
2312

Line->Level is an unsigned int so this gives me a warning. I guess it should be unsigned int OriginalLevel same below

switch function to unsigned

JohelEGP requested changes to this revision.Thu, Jun 25, 6:47 PM
JohelEGP added a subscriber: JohelEGP.

BreakBeforeBraces: Allman isn't respected.

template <class D, class U, class Rep2>
friend constexpr auto
operator*(const size2d& l, const units::quantity<D, U, Rep2>& r) noexcept(
    noexcept(l.w* r)) requires(requires { l.w* r; }) //
{
    return size2d<decltype(l.w() * r)>{l.w * r, l.h * r};
}

When a trailing requires-clause is right before the function body, its opening brace should go on its own line, as dictated by my config file.

This revision now requires changes to proceed.Thu, Jun 25, 6:47 PM
curdeius added inline comments.
clang/include/clang/Format/Format.h
533

It would be nice to have an example here in the doc.

clang/unittests/Format/FormatTest.cpp
13642

Format: leading whitespace. Should it be in the same "group" as AlignConsecutiveMacros?

16906

Isn't it a strange indentation?

constexpr plane(const plane& other) noexcept(
    (detail::default_allocator_is_nothrow &&
     std::is_nothrow_copy_constructible_v<T>)) requires(std::copyable<T>)
  : plane{to1d(other), other.sz.w}
{
}

Without the parentheses in trailing requires-clauses, all code following the constructor initializer list is further indented.

MyDeveloperDay marked 3 inline comments as done.Wed, Jul 1, 3:55 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
13642

I might be misunderstanding your comment, but the list should alphabetic but now I realize it should be after the Allows

16906

there is no newline on the line above, but I clang-format the tests so it wraps it, I know visually its confusing

MyDeveloperDay marked an inline comment as done.Wed, Jul 1, 3:56 AM
constexpr plane(const plane& other) noexcept(
    (detail::default_allocator_is_nothrow &&
     std::is_nothrow_copy_constructible_v<T>)) requires(std::copyable<T>)
  : plane{to1d(other), other.sz.w}
{
}

Without the parentheses in trailing requires-clauses, all code following the constructor initializer list is further indented.

Just so I'm clear for this and your Allman coment would you show me what you are seeing and what you expect to see? just so I understand.

Just so I'm clear for this and your Allman coment would you show me what you are seeing and what you expect to see? just so I understand.

Sure, thank you.

This is what I have, as shown above. Actually fixed to show surrounding context, which is relevant to formatting. Note the // comment:

namespace jge
{
template <std::regular Rep>
struct [[nodiscard]] size2d
{
    template <class D, class U, class Rep2>
    friend constexpr auto
    operator*(const size2d& l, const units::quantity<D, U, Rep2>& r) noexcept(
        noexcept(l.w* r)) requires(requires { l.w* r; }) //
    {
        return size2d<decltype(l.w() * r)>{l.w * r, l.h * r};
    }
};

} // namespace jge

I added // to make the formatting look like what I want. If I remove it, this is what I get:

namespace jge
{
template <std::regular Rep>
struct [[nodiscard]] size2d
{
    template <class D, class U, class Rep2>
    friend constexpr auto
    operator*(const size2d& l, const units::quantity<D, U, Rep2>& r) noexcept(
        noexcept(l.w* r)) requires(requires { l.w* r; }) {
        return size2d<decltype(l.w() * r)>{l.w * r, l.h * r};
    }
};

} // namespace jge

So this is what I want, without the // comment:

namespace jge
{
template <std::regular Rep>
struct [[nodiscard]] size2d
{
    template <class D, class U, class Rep2>
    friend constexpr auto
    operator*(const size2d& l, const units::quantity<D, U, Rep2>& r) noexcept(
        noexcept(l.w* r)) requires(requires { l.w* r; }) 
    {
        return size2d<decltype(l.w() * r)>{l.w * r, l.h * r};
    }
};

} // namespace jge

For the constructor initializer list, this is what I have. Note the parentheses in the first requires-clause:

namespace jge
{
template <class T>
class [[nodiscard]] plane
{
    constexpr plane(const plane& other) noexcept(
        (detail::default_allocator_is_nothrow &&
         std::is_nothrow_copy_constructible_v<T>)) requires(std::copyable<T>)
      : plane{to1d(other), other.sz.w}
    {
    }

    constexpr plane& operator=(const plane& other) noexcept(
        (std::is_nothrow_copy_constructible_v<plane> &&
         std::is_nothrow_copy_assignable_v<T>)) requires std::copyable<T> //
    {
        assign(to1d(other), other.sz.w);
        return *this;
    }
};

} // namespace jge

This is what I get without the parentheses. Note that all code after the constructor initializer list is further indented:

namespace jge
{
template <class T>
class [[nodiscard]] plane
{
    constexpr plane(const plane& other) noexcept(
        (detail::default_allocator_is_nothrow &&
         std::is_nothrow_copy_constructible_v<T>)) requires std::copyable<T>
      : plane{to1d(other), other.sz.w}
      {
      }

      constexpr plane& operator=(const plane& other) noexcept(
          (std::is_nothrow_copy_constructible_v<plane> &&
           std::is_nothrow_copy_assignable_v<T>)) requires std::copyable<T> //
      {
          assign(to1d(other), other.sz.w);
          return *this;
      }
};

} // namespace jge

So this is what I want. Without extra parentheses, nor extra indentation:

namespace jge
{
template <class T>
class [[nodiscard]] plane
{
    constexpr plane(const plane& other) noexcept(
        (detail::default_allocator_is_nothrow &&
         std::is_nothrow_copy_constructible_v<T>)) requires std::copyable<T>
      : plane{to1d(other), other.sz.w}
    {
    }

    constexpr plane& operator=(const plane& other) noexcept(
        (std::is_nothrow_copy_constructible_v<plane> &&
         std::is_nothrow_copy_assignable_v<T>)) requires std::copyable<T> //
    {
        assign(to1d(other), other.sz.w);
        return *this;
    }
};

} // namespace jge

All with the config file linked above.

Thank you I see now... super subtle but I'll take a look.

Using parameter packs within non-parenthesized requires-clauses results in weird formatting:

template <std::semiregular F, std::semiregular... Args>
    requires std::invocable < F, std::invoke_result_t<Args>
... > struct constant;

vs

template <std::semiregular F, std::semiregular... Args>
    requires(std::invocable<F, std::invoke_result_t<Args>...>)
struct constant;

Still a work in progress but covers I think the case @JohelEGP found.

curdeius marked an inline comment as done.Thu, Jul 2, 1:51 PM
curdeius added inline comments.
clang/unittests/Format/FormatTest.cpp
13642

I just wanted to point out that when you don't put a blank line after CHECK_PARSE_BOOL, then the next one gets indented. And yes, the order isn't ok. BTW, us "Always" in the name necessary? Cf. other "Break" options.

16906

O, ok, thanks for the explanation. It's indeed confusing in the review.

MyDeveloperDay marked an inline comment as done.Fri, Jul 3, 2:05 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
13642

@curdeius I didn't get to make changes to match your previous review comments yet, but I 100% agree with you why did I put Always here!

I WILL remove that as I can see further down the road if this got changed to an enumeration it would be

AlwaysBreakBeforeConceptDeclarations = Always
AlwaysBreakBeforeConceptDeclarations = Sometimes
AlwaysBreakBeforeConceptDeclarations = Never

Which would be very confusing...

I'll address those concerns next

MyDeveloperDay marked 4 inline comments as done.

Addressed review comments
Added one or two more tests

NOTE: renamed the option from AlwaysBreakBeforeConceptDeclarations -> BreakBeforeConceptDeclarations
JohelEGP requested changes to this revision.Sat, Jul 4, 8:17 PM

Thank you. Everything I reported works fine now.

I have two more cases for now. First are your examples in the OP.
You have

template <typename T>
concept bool EqualityComparable = requires(T a, T b) {
  { a == b } -> bool;
};

But this is what I get:

template <typename T>
concept bool EqualityComparable = requires(T a, T b)
{
    {
        a == b
        } -> bool;
};

Perhaps it's because I compiled against commit 71d88cebfb42c8c5ac2d54b42afdcca956e55660 (Fri Jul 3 13:46:41 2020 -0400).
Next:

template <std::semiregular F, std::semiregular... Args>
    requires std::invocable<F, std::invoke_result_t<Args>...>
struct [[nodiscard]] constant : std::bool_constant < requires
{
    typename _require_constant_<(std::invoke(F{}, Args{}()...), 0)>;
} > {};

There's spaces around the template brackets. The lack of indentation is also odd, considering that in this context things are indented.

This revision now requires changes to proceed.Sat, Jul 4, 8:17 PM

Another weird formatting when direct-initializing a variable with a non-parenthesized requires-expression:

constexpr void test_width()
{
    bool invalid_rep{!requires {typename jge::width<T>;
}
}
;
}

vs

constexpr void test_width()
{
    bool invalid_rep{!(requires { typename jge::width<T>; })};
    bool invalid_rep = !(requires { typename jge::width<T>; });
    bool invalid_rep = !requires
    {
        typename jge::width<T>;
    };
}

Thank you @JohelEGP please keep them coming, I'll try and update the patch as soon as I have some fixes, I kind of feel like we are addressing what would have just been downstream bugs anyway. (some are quite bizarre!) so lets squish as many of them as we can before we land this.

My assumption is you are giving me valid c++ code, if so that is great I'll try and address them, but your examples are probably beyond my own concepts knowledge. (This is actually why I started to add this to clang-format, I wanted a way to learn about concepts)

MyDeveloperDay added a comment.EditedTue, Jul 7, 12:48 AM

https://reviews.llvm.org/D79773#2131680 has something to do with your .clang-format file, the base LLVM style seems fine.

template <typename T>
concept bool EqualityComparable = requires(T a, T b) {
    { a == b } -> bool;
};

vs

template <typename T>
concept bool EqualityComparable = requires(T a, T b)
{
    {
        a == b
        } -> bool;
};

From what I can tell its because of the "BreakBeforeBraces: Allman or GNU" styles.

I'm wondering what an Allman version should look like?

template <typename T>
concept bool EqualityComparable = requires(T a, T b)
{
    {
        a == b
     } -> bool;
};
JohelEGP added a comment.EditedWed, Jul 8, 12:32 AM

Ah, that makes sense. The very definition of Allman is to always break before braces. I suppose I'll need to use BreakBeforeBraces: Custom and BraceWrapping:. I did some testing and noticed that the weird format comes with any of these:

BreakBeforeBraces: Custom
BraceWrapping:
  AfterControlStatement: MultiLine
BreakBeforeBraces: Custom
BraceWrapping:
  AfterFunction: true

Since a compound requirement is neither a control statement nor a function, I suppose it might eventually need its own BraceWrapping nested configuration flag. For now, I'd prefer if they never break.

MyDeveloperDay added a subscriber: klimek.EditedThu, Jul 9, 1:54 AM

This issue is caused by the presence of the ; in the requires clause (0)>;) inside the template declaration

template <std::semiregular F, std::semiregular... Args>
    requires std::invocable<F, std::invoke_result_t<Args>...>
struct [[nodiscard]] constant : std::bool_constant < requires
{
    typename _require_constant_<(std::invoke(F{}, Args{}()...), 0)>;
} > {};

These below may not be legal C++ examples but show the same behaviour

struct constant : Foo < typename {
    Foo;
} > {};

struct constant : Foo<typename {Foo}> { };

This is actually failing in the handling of parsing the <> in (parseAngle() actually parseBrace() inside of that), it is not really concepts specific except it may be seen now because of the presence of the ;

The problem comes about I think because the line is broken by the ; during the parseBrace at some point the Next token is null, I assume that's because the lines are broken by the ; as such ultimately the < and > rather than being seen as TT_TemplateOpener and TT_TemplateCloser are seen as TT_BinaryOperators (by default when 'parseAngle' fails)

I'm not sure how I could solve this, I might need help from a higher power @klimek

klimek added inline comments.Thu, Jul 9, 4:32 AM
clang/lib/Format/UnwrappedLineParser.cpp
631–636

I'd have expected this to be just in front of line 629 (if... tok::semi) - does that break something? Don't we want to munch the semi after this?

2336

This seems like a very detailed way to parse them; generally, we try to only parse rough structure here.