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

[range.utility.conv.general] Should reservable-container and container-insertable be concepts? #5328

Closed
hewillk opened this issue Mar 2, 2022 · 11 comments
Closed

[range.utility.conv.general] Should reservable-container and container-insertable be concepts? #5328

hewillk opened this issue Mar 2, 2022 · 11 comments

Comments

@hewillk
Copy link
Contributor

hewillk commented Mar 2, 2022 •
edited
Loading

In [range.utility.conv.general], reservable-container and container-insertable are defined as

template<class Container>
constexpr bool reservable-container =          // exposition only
  sized_­range<Container> &&
  requires(Container& c, range_size_t<Container> n) {
    c.reserve(n);
    { c.capacity() } -> same_­as<decltype(n)>;
    { c.max_size() } -> same_­as<decltype(n)>;
  };

template<class Container, class Ref>
constexpr bool container-insertable =          // exposition only
  requires(Container& c, Ref&& ref) {
    requires (requires { c.push_back(std::forward<Ref>(ref)); } ||
              requires { c.insert(c.end(), std::forward<Ref>(ref)); });
  };

I don't understand why they lack inline, and it seems to me that defining them as concept is more consistent with the name, since the inline constexpr bool variables usually start with is_.
@hewillk hewillk changed the title [range.utility.conv.general] Should reservable-container and container-insertable be concepts? [range.utility.conv.general] Should _reserve-container_ and _container-insertable_ be concepts? Mar 2, 2022
@hewillk hewillk changed the title [range.utility.conv.general] Should _reserve-container_ and _container-insertable_ be concepts? [range.utility.conv.general] Should reserve-container and container-insertable be concepts? Mar 2, 2022
@JohelEGP
Copy link
Contributor

JohelEGP commented Mar 2, 2022

I agree that prepending is- results in a better name, unless they're made concepts. These don't need to be inline since their address is not leaked, and thus the difference is not observable.

@hewillk
Copy link
Contributor Author

hewillk commented Mar 2, 2022

I know, but exposition only constexpr bool in <ranges> all are inline. Should we be consistent with them?

@JohelEGP
Copy link
Contributor

JohelEGP commented Mar 2, 2022

That makes sense.

@hewillk
Copy link
Contributor Author

hewillk commented Mar 2, 2022

In the original paper p1206r7, the description of reservable-container is

reservable-container concept
The exposition only reservable-container concepts checks for
• reserve
• max_size
• capacity
which is consistent with ranges-v3. This serves multiple purposes:
• Avoid calling reserve on unordered containers and others entities that have a reserve
method with different semantics
• Allow implementations to check that we are not trying to reserve more than max size

So I suspect it should be a concept in nature, and the opinion of the author of the paper may be required.

@jwakely
Copy link
Member

jwakely commented Mar 2, 2022

No, they should not be inline. They don't actually exist, so we don't need to add noise to the declaration to avoid linker errors for things that aren't really there.

No, we don't want them to be concepts. They were concepts in an earlier revision of the paper and it was changed intentionally by LWG review. The reason it's a variable template not a concept is because we don't want the semantic requirements of the constraints to result in UB if violated. It's a purely syntactic check.

@jwakely
Copy link
Member

jwakely commented Mar 2, 2022 •
edited
Loading

I know, but exposition only constexpr bool in <ranges> all are inline. Should we be consistent with them?

Maybe we should remove it from them too.

Looks like this affects is-derived-from-view-interface and is-initializer-list specifically.

@jensmaurer
Copy link
Member

No, they should not be inline, because variable templates are implicitly inline.

@JohelEGP
Copy link
Contributor

JohelEGP commented Mar 2, 2022

Indeed, see #4625.

@hewillk
Copy link
Contributor Author

hewillk commented Mar 6, 2022 •
edited
Loading

Do we need to rename container-insertable to insertable-container to make it consistent with the naming of reservable-container? It seems a bit odd to me that two similar concepts have different name styles.
This also makes it consistent with output_iterator which has a similar requires-clause style, which is also a noun rather than an adjective.

@hewillk hewillk changed the title [range.utility.conv.general] Should reserve-container and container-insertable be concepts? [range.utility.conv.general] Should reservable-container and container-insertable be concepts? Mar 6, 2022
@jensmaurer
Copy link
Member

No. reservable-container is a property of a container (regardless of element type etc), but container-insertable is about some Ref that wants to be inserted in some container, so it talks about something in relationship to a container. Different naming styles are fine.

@jensmaurer
Copy link
Member

I'm not seeing any more open issues here. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@jwakely @JohelEGP @jensmaurer @hewillk