Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[feature request] requires clause in functions (concepts TS) #31514

Open
gnzlbg mannequin opened this issue Mar 7, 2017 · 7 comments
Open

[feature request] requires clause in functions (concepts TS) #31514

gnzlbg mannequin opened this issue Mar 7, 2017 · 7 comments
Labels
bugzilla Issues migrated from bugzilla clang-format

Comments

@gnzlbg
Copy link
Mannequin

gnzlbg mannequin commented Mar 7, 2017 •
edited by rymiel

Bugzilla Link 32166
Version trunk
OS All
CC @CaseyCarter,@HazardyKnusperkeks,@mydeveloperday

Extended Description

This code

template <bool IsConst>
struct A {
  void prev() 
  requires BidirectionalIterator<iterator_t<IsConst>>() &&
           BidirectionalIterator<iterator_t<IsConst>>() &&
           BidirectionalIterator<iterator_t<IsConst>>() 
  { --it_; }
};

gets formatted into:

template <bool IsConst>
struct A {
  void prev() requires BidirectionalIterator<iterator_t<IsConst>>() &&
      BidirectionalIterator<iterator_t<IsConst>>() &&
      BidirectionalIterator<iterator_t<IsConst>>() {
    --it_;
  }
};

I don't see a reason why the options specified in #​32165 shouldn't apply here as well.

Also: https://github.com/CaseyCarter/cmcstl2/blob/43b63f6846d80bcd1867f512519bc18841bd961e/include/stl2/detail/iterator/operations.hpp#L25

this code:

template <class I>
requires
        Iterator<I>()
        // Pre: 0 <= n && [i,i+n)
constexpr void impl(I& i, difference_type_t<I> n)
noexcept(noexcept(++std::declval<I&>()))
{
    STL2_EXPECT(0 <= n);
    while (n != 0) {
        --n;
        ++i;
    }
}

gets formatted into

template <class I>
requires Iterator<I>()
    // Pre: 0 <= n && [i,i+n)
    constexpr void impl(I& i, difference_type_t<I> n) noexcept(
        noexcept(++std::declval<I&>())) {
  STL2_EXPECT(0 <= n);
  while (n != 0) {
    --n;
    ++i;
  }
}

(Note how the constexpr keywords is not aligned with the template keyword).
@gnzlbg
Copy link
Mannequin Author

gnzlbg mannequin commented Mar 7, 2017

assigned to @HazardyKnusperkeks

@CaseyCarter
Copy link
Member

Ditto Poke, poke. Now that C++20 has been sent out for (what will likely be) its final round of balloting, it would be a good time to work on supporting requires-clauses in clang-format. Think of all the poor, suffering STL maintainers!

@mydeveloperday
Copy link
Contributor

I've started to look into the concepts and requires keywords and how clang-format interacts with them (its doesn't really)

I'd like @​Casey to start with a slightly simpler example (if we could start without the comment that might help)

For the following would you expect it to be formatted like this?

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

or something else? I'm trying to look for something of a common starting place between what I see in both

https://github.com/CaseyCarter/cmcstl2
and
https://github.com/microsoft/STL

@CaseyCarter
Copy link
Member

I've started to look into the concepts and requires keywords and how
clang-format interacts with them (its doesn't really)

I'd like @​Casey to start with a slightly simpler example (if we could start
without the comment that might help)

For the following would you expect it to be formatted like this?

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

Today, I'd format this as:

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

with the requires-clause indented once relative to the template declaration, and continuation lines one indent deeper:

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

which probably follows from how clang normally wraps expressions across lines. I try to format trailing requires-clauses exactly as you've put the noexcept-specifier in the example, but wrapping the whole thing onto the next line if it won't fit on the primary declaration line and only splitting if the requires-clause itself is longer than one line:

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

template
long_return_type g(I i)
requires Concept1 && Concept2 && VeryVeryVeryVeryLongConcept {
// ...
}

template
long_return_type h(I i) requires Concept1 && Concept2 &&
VeryVeryVeryVeryLongConcept && OtherLongConcept {
// ...
}

or something else? I'm trying to look for something of a common starting
place between what I see in both

https://github.com/CaseyCarter/cmcstl2
and
https://github.com/microsoft/STL

Please ignore cmcstl2: there a few different styles there that we experimented with over the years, and it was never really made consistent. range-v3 is even worse: its style represents a set of compromises to get the least offensive output from clang-format. I have no idea how libstdc++ is formatting their concepts usage, but they have C++20 Ranges implemented so they may have an interesting counterpoint to my style.

@HazardyKnusperkeks
Copy link
Contributor

I'm working on it.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
@llvmbot llvmbot added the confirmed Verified by a second party label Jan 26, 2022
@HazardyKnusperkeks
Copy link
Contributor

HazardyKnusperkeks commented Feb 5, 2022 •
edited

The review link would be: https://reviews.llvm.org/D113319
But I have decided that the style

template <typename T>
requires
         C<T>
class ...

will not be supported. If you are really interested in it you can look at some older revision where I had a basic handling of that and try to get it working.

@HazardyKnusperkeks HazardyKnusperkeks added awaiting-review Has pending Phabricator review and removed awaiting-review Has pending Phabricator review confirmed Verified by a second party labels Feb 5, 2022
@HazardyKnusperkeks
Copy link
Contributor

Please update this, either as solved, or what is still wished and we can discuss that.

@HazardyKnusperkeks HazardyKnusperkeks removed their assignment Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bugzilla Issues migrated from bugzilla clang-format
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@CaseyCarter @mydeveloperday @HazardyKnusperkeks @llvmbot