This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Improve clang-formats handling of concepts
ClosedPublic

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

MyDeveloperDay created this revision.May 12 2020, 3:06 AM
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
2002

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
1624โ€“1628

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

3601

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
2309

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
1624โ€“1628

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;

3601

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
2309

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
3610

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
3610

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
1624โ€“1628

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

3601

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
2309

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
2306

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
2337

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
2337

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
17194

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
1624โ€“1628

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
2306

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

2337

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
2313

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.Jun 25 2020, 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.Jun 25 2020, 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
14098

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

17466

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.Jul 1 2020, 3:55 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
14098

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

17466

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.Jul 1 2020, 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.Jul 2 2020, 1:51 PM
curdeius added inline comments.
clang/unittests/Format/FormatTest.cpp
14098

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.

17466

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

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

@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.Jul 4 2020, 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.Jul 4 2020, 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.EditedJul 7 2020, 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.EditedJul 8 2020, 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.EditedJul 9 2020, 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.Jul 9 2020, 4:32 AM
clang/lib/Format/UnwrappedLineParser.cpp
638โ€“643

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?

2337

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

I found another case. See how everything after the requires-clause is indented for constructors with just the constructor's name (it works otherwise, maybe because it looks like a function).

class [[nodiscard]] data_t
{
public:
    template <std::ranges::input_range R>
        requires std::same_as<std::ranges::range_value_t<R>, flippable_tile_id>
        /*explicit*/ data_t(R&& ids, const format_t f = encoding_t::csv)
          : format_t{f}, tile_ids_{ranges::to_vector(std::forward<R>(ids))}
        {
        }

        [[nodiscard]] bool operator==(const data_t&) const = default;
};

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 ;

I opened LLVM 47161 to defer this to upstream.

Rheel added a subscriber: Rheel.Aug 17 2020, 6:47 AM

Could you do a rebase? The patch no longer applies:

error: patch failed: clang/docs/ClangFormatStyleOptions.rst:2711
error: clang/docs/ClangFormatStyleOptions.rst: patch does not apply
error: patch failed: clang/docs/ReleaseNotes.rst:398
error: clang/docs/ReleaseNotes.rst: patch does not apply
error: patch failed: clang/lib/Format/FormatToken.h:470
error: clang/lib/Format/FormatToken.h: patch does not apply
error: patch failed: clang/lib/Format/TokenAnnotator.cpp:1337
error: clang/lib/Format/TokenAnnotator.cpp: patch does not apply

Rebase the patch

njakob added a subscriber: njakob.Oct 25 2020, 8:19 AM

@MyDeveloperDay, is there anything I can do to help you on that?

BTW, libc++ would like to use clang-format (more than it does currently) and having concepts support would be awesome as there are more and more concept-related patches coming in.
IMHO, even support for most common cases would be a great improvement.

I don't think it was far off, I just I agreed with @klimek in trying to address the many different cases that came up I started to feel that parseConstraintExpression was becoming fragile

Having said that I like to think because they are in concepts specific parse code I won't break too much (famous last words!), any if you've tried to format concept code with the current clang-format well good luck with that.

Perhaps something is better than nothing?

if you think it has merits, if it covers most cases ok, then I'd be happy to see it land and work on any refinements from there.

It would be maybe possible to:

  • address @klimek 's comment and do any other necessary cleanup
  • create a bug ticket for what @JohelEGP found with ctors
  • mark clang-format's support of concept as experimental?

This patch is getting big IMO and I really think that something is better than nothing in this case.
And at least we could parallelise the efforts on the remaining (edge) cases when this gets landed.
WDYT?

MyDeveloperDay marked an inline comment as done.

rebase before making any further changes

MyDeveloperDay marked an inline comment as done.Dec 3 2020, 1:12 PM
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
2337

So whilst I tend to agree, and I've tried to write this loop to be less and it just didn't seem to catch the cases that had already been given in the unit tests,

This ended up being a lot likes its own parseStructuralElement, I think the difference here is that I've written it as a series of if's rather than a switch statement.

I feel I'd be happier to push the patch through then address individual specific cases

MyDeveloperDay marked an inline comment as done.

Address the issue with still munching the semi
mark as experimental in the release notes

Having reviewed the current status I'm going to leave this patch with just minor changes from where its been for some time, if others feel it has merits in landing as experimental support I'd be happy with that. I would rather build from a base than try and keep rebuilding the same tower.

The concepts support in clang-format is currently poor, actually, it's downright destructive, I've tried to cover the basic concept usage and I'm happy to support this feature against individual bug report where I can.

miscco added a comment.Dec 3 2020, 1:32 PM

As someone who has extensivly worked with conscepts I cannot stress how much this would improve my live

As someone who has extensively worked with concepts I cannot stress how much this would improve my live

and I'd like to think that this will help even in its current form, but it needs someone to accept it first before it can land,

This has been my problem with clang-format , I do try and give some of my time to keeping up with the new and outstanding reviews coming in, but my own progress on clang-format or the fixing of bugs has slowed up because of a lack of people to review, we just need a couple more people who are willing to come once or twice a week and just keep things rolling along. but someone has to "watch the watcher" too.

mitchell-stellar accepted this revision.Dec 4 2020, 6:18 AM

I am in favor of landing this and iterating.

curdeius accepted this revision.Dec 4 2020, 6:27 AM

LGTM.

miscco accepted this revision.Dec 4 2020, 6:56 AM

@JohelEGP can you let me know if you plan to remove the "requested changes" you asked for back in July?

I cannot commit with the revision until that is removed

This revision was not accepted when it landed; it landed in state Needs Review.Dec 4 2020, 9:46 AM
This revision was automatically updated to reflect the committed changes.

I was a bit late, but thanks for everything!

I was a bit late, but thanks for everything!

No problem, sorry not for waiting, but let the games begin.

P.S. I think I may have left a comment for you over in one of your bugs. not sure if you saw that