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.

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

Use consistent wording to indicate ADL-only lookup. #3216

Closed
wants to merge 2 commits into from

Conversation

Quuxplusone
Copy link
Contributor

Related to "[isocpp-lib] ADL wording formula" and LWG3247; attn @CaseyCarter and @W-E-Brown among others.

This is intended to be basically editorial. The intent is:

  • to use the same wording in each affected place,
  • to make sure the words "ADL" are used with a crossreference to the section on ADL,
  • to eliminate references to "poison-pill" deleted overloads which generally are not needed, not even as an implementation detail, post-C++17.

(For example, ranges::swap doesn't need a poison pill because std::swap is properly constrained ever since C++17.)

Posting to gather wording feedback, not because I would expect this to get merged right away.
In particular, there's a whole paragraph in "lib-intro.tex" that is basically obsolete at this point, because all it's saying now is "when we say to look up a name by ADL only, we mean to look up a name by ADL only" — it doesn't have to explain the trick because there's no longer any trick. Maybe that paragraph could just be removed.

\end{codeblock}
and does not include a declaration of \tcode{ranges::swap}.
has class or enumeration type\iref{basic.compound} and that expression is valid,
when `swap` is looked up using only argument-dependent lookup\iref{basic.lookup.argdep}.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The \note is probably unnecessary; we have no such notes in other places.

I'm not happy grammatically with the phrase "X is equivalent to Y, if Y is valid when Z." What we mean is more like "X is equivalent to Y-when-Z, if Y is valid when Z."

valid expression
when `begin` is looked up using only argument-dependent lookup\iref{basic.lookup.argdep}
and its type \tcode{I} models
\libconcept{input_or_output_iterator}.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of these ranges customization points end with the formula "...and its type I models some-concept." I don't understand why we need to introduce the name I, given that we never use the name I again in this scope. However, I didn't change these formulas in this patch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's because [customization.point.object]p5 requires it, although some CPOs don't do that, IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand this answer, btw. My guess is that you were answering the unasked question "Why does the formula mention that the type models some-concept?", but not "Why does the formula introduce the name I to refer to a type that is never mentioned again?"

I mean, it seems like we should just say "...and its type models some-concept." Interjecting the unused identifier I seems unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The placeholder type names are probably leftover from an earlier "and its type X models Concept<X>" formulation that was only half cleaned up; they serve no purpose now. I'd replace "valid expression and its type X models Concept" with "valid expression whose type models Concept" throughout.

My hunch is that the "...except that [ADL]" wording should bind tightly to the
expression, since we mean that the [expression-except-ADL] is both the
thing we're SFINAEing on *and* the thing we're eventually going to
evaluate.

Also \tcode{} instead of backticks, oops.
@jensmaurer
Copy link
Member

It's not at all clear to me that this change is editorial (i.e. not changing semantics for corner cases), so (personally), I'd prefer a paper approved by LWG. Specifically, could you elaborate why the deleted overloads were necessary before, but no longer so?

Further, I'm not sure that "only ADL" is implementable in plain C++ (which, of course, does not necessarily constrain the standard library). For example, given

int strong_order(X,Y);
#include <stdlib_header>

it seems that strong_order may have been found previously (since unqualified lookup inside stdlib_header may eventually reach the global scope), but now you're not supposed to find that unless X or Y are declared in global scope.

@Quuxplusone
Copy link
Contributor Author

Specifically, could you elaborate why the deleted overloads were necessary before, but no longer so?

My understanding (and maybe @ericniebler and/or @CaseyCarter could weigh in with better understanding) is that the "poison-pill" overloads in range-v3 are needed because range-v3 is a C++14-compatible library. In C++14, std::swap (for example) was an unconstrained template.[*] Adding a new deleted overload was needed in order to stop std::mutex m1, m2; std::ranges::swap(m1, m2); from accidentally dispatching to std::swap<std::mutex> and producing a hard error instead of SFINAE-friendliness. In C++17, std::swap<std::mutex> does not participate in overload resolution, so we do not worry that the ADL-only lookup of swap(m1, m2) might choose it.

Further, I'm not sure that "only ADL" is implementable in plain C++

This seems very worrisome if true! But I don't understand your example. Here's my attempt to reproduce it with swap as an example: https://ranges.godbolt.org/z/zRUvCb
The implementation strategy for "ADL-only lookup" remains the same: the library implementor puts some untenable overload of swap (or strong_order) somewhere in the detail namespace that actually makes the call, so that unqualified lookup finds that untenable thing and stops before finding std::swap. It's just that in C++17, the untenable declaration doesn't need to have precisely the specified poison-pill form; it might be as simple as int swap();.
Example of what I'm thinking: https://godbolt.org/z/AEdYPd

[*] — That is, according to cppreference. According to libc++, it's been constrained since 2011. According to N4140 (C++14), it was constrained but only via SFINAE in its noexcept clause — admittedly too subtle.

@jensmaurer
Copy link
Member

jensmaurer commented Sep 19, 2019

Thanks for the explanation. It seems the "int swap()" poison-pill is actually better than the previous form, because it stops non-ADL name lookup, but otherwise is non-viable and doesn't interfere with overload resolution. Consider https://ranges.godbolt.org/z/2VMmyq :
Obviously, my template swap in namespace N is found via ADL (status quo of the example), however, ranges::swap doesn't find it (remove comment marker to see it). At least this implementation seems to violate the "nothing but ADL" rule.
Even if my template swap were found, it would produce an overload ambiguity with the hitherto-prescribed poison pill, whereas the new wording doesn't. That at least seems like a normative (i.e. non-editorial) change to me.

@jensmaurer jensmaurer added the decision-required A decision of the editorial group (or the Project Editor) is required. label Sep 19, 2019
@Quuxplusone
Copy link
Contributor Author

Quuxplusone commented Sep 19, 2019

Consider https://ranges.godbolt.org/z/2VMmyq : Obviously, my [completely unconstrained] template swap in namespace N is found via ADL (status quo of the example), however, ranges::swap doesn't find it [...]

Hmm, good point. This change is far from editorial (but still, IMO, good! ;)) and needs a defect/paper. At the very least, your example shows that the current explanatory wording in [customization.point.object] is incorrect:

To preclude such an expression resulting in a call to unconstrained functions [sic]
with the same name in namespace \tcode{std}, [...]

The currently specified poison-pill also blocks unconstrained function templates with the same name in associated namespaces! Which, as you say, means that the current poison-pill approach is not the same thing as "ADL-only lookup."

Also, [customization.point.object] means "unconstrained" in the literal C++2a-lawyer sense. For example, this template is considered "unconstrained" by C++2a and by Ranges-V3, even though someone coming from C++17 would probably have said colloquially that it was "constrained" to accept only Ts derived from N::A.

template<class T>
std::enable_if_t<std::is_base_of<N::A, T>::value>
swap(T&, T&) { /* something */ }

One might plausibly argue that users who write completely unconstrained, C++03-swap-style, templates deserve what they get — but I don't think we should ship a ranges::swap that claims to do ADL-only lookup yet quietly ignores this kind of "colloquially constrained" ADL template.

@jensmaurer
Copy link
Member

We seem to agree the changes are not editorial. Could I kindly ask you to take the matter to LWG via their reflector and/or via their issues list?

@jensmaurer jensmaurer added lwg Issue must be reviewed by LWG. and removed decision-required A decision of the editorial group (or the Project Editor) is required. labels Sep 19, 2019
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the ADL wording changes, but removing the poison pills is normative and needs some non-editor eyes.

\tcode{S} is \tcode{(void)swap(E1, E2)},
except that \tcode{swap} is looked up using only argument-dependent lookup\iref{basic.lookup.argdep},
if that expression is well-formed when treated as an unevaluated operand and
if \tcode{E1} or \tcode{E2} has class or enumeration type\iref{basic.compound}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Removing the poison pill has normative effect - it allows a function template template<class T> void swap(T&, T&) defined in an associated namespace to be selected by overload resolution. I urge you to leave the poison pill alone in this PR and submit a separate LWG issue to remove it.

  • I agree with your assessment that this is awkward. How about:

    \tcode{S} is \tcode{(void)swap(E1, E2)},
      if \tcode{E1} or \tcode{E2} has class or enumeration type\iref{basic.compound}
      and that expression is well-formed 
      when treated as an unevaluated operand 
      in which \tcode{swap} is looked up using only 
      argument-dependent lookup\iref{basic.lookup.argdep}.

    (with similar changes to the following occurrences, of course)?

void iter_swap(I1, I2) = delete;
\end{codeblock}
and does not include a declaration of \tcode{ranges::iter_swap}.
\item \tcode{(void)iter_swap(E1, E2)},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the poison pill here has normative effect - it allows the unconstrained std::iter_swap to be selected by overload resolution. I'd urge you not to submit an LWG issue to remove it.

``is looked up using only argument-dependent lookup\iref{basic.lookup.argdep}.''
Functions in namespace \tcode{std} having that name will not be found
by such lookup unless \tcode{std} is an associated namespace
of the function call expression.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note was useful in the Ranges TS when it was a page away from the specification of the CPOs, but it's useless in the IS and moreso after applying this PR. Let's just strike it.

valid expression
when `begin` is looked up using only argument-dependent lookup\iref{basic.lookup.argdep}
and its type \tcode{I} models
\libconcept{input_or_output_iterator}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The placeholder type names are probably leftover from an earlier "and its type X models Concept<X>" formulation that was only half cleaned up; they serve no purpose now. I'd replace "valid expression and its type X models Concept" with "valid expression whose type models Concept" throughout.

\end{codeblock}

and does not include a declaration of \tcode{ranges::begin}.
Otherwise, \tcode{\placeholdernc{decay-copy}(begin(E))},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto "Removing the poison pill(s) is not editorial and also not a good idea."

@@ -367,19 +362,13 @@
\end{codeblock}

\item
Otherwise, \tcode{\placeholdernc{decay-copy}(end(E))} if it is a valid
expression and its type \tcode{S} models
Otherwise, \tcode{\placeholdernc{decay-copy}(end(E))},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto "Removing the poison pill(s) is not editorial and also not a good idea."

\end{codeblock}

and does not include a declaration of \tcode{ranges::rbegin}.
Otherwise, \tcode{\placeholdernc{decay-copy}(rbegin(E))},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto "Removing the poison pill(s) is not editorial and also not a good idea."

@@ -494,18 +478,13 @@
\end{codeblock}

\item
Otherwise, \tcode{\placeholdernc{decay-copy}(rend(E))} if it is a valid
expression and its type \tcode{S} models
Otherwise, \tcode{\placeholdernc{decay-copy}(rend(E))},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto "Removing the poison pill(s) is not editorial and also not a good idea."

\end{codeblock}

and does not include a declaration of \tcode{ranges::size}.
Otherwise, \tcode{\placeholdernc{decay-copy}(size(E))},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the poison pill is not editorial, but probably a good idea.

@CaseyCarter
Copy link
Contributor

Which, as you say, means that the current poison-pill approach is not the same thing as "ADL-only lookup."

We're trying to express that overload resolution is performed on a set of functions and function templates that includes the poison pill(s) as well as the results of ADL-only name lookup.

resolution performed in a context that includes the declarations:
\begin{codeblock}
template<class T> void begin(T&&) = delete;
template<class T> void begin(initializer_list<T>&&) = delete;
Copy link
Contributor

@brevzin brevzin Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to elaborate on @CaseyCarter's comments here in case there's some ambiguity. Removing this poison pill is not editorial and also a bad idea. Consider the following type:

namespace N {
    struct my_vector { /* ... */ };
    int const* begin(my_vector const&);
    int const* end(my_vector const&);
}

It's the poison pill that prevents N::my_vector from satisfying forwarding-range, removing them would open up some unknowable number of containers to accidentally opt-in to this concept when they definitely should not.

@jensmaurer
Copy link
Member

@Quuxplusone, is there some editorial cleanup/harmonization left that you wish to have applied given @CaseyCarter's comments, or is this all LWG material?

@jensmaurer jensmaurer added changes requested Changes to the wording or approach have been requested and not yet applied. needs rebase The pull request needs a git rebase to resolve merge conflicts. labels Oct 4, 2019
@jensmaurer
Copy link
Member

@Quuxplusone, ping?

@Quuxplusone
Copy link
Contributor Author

@Quuxplusone, is there some editorial cleanup/harmonization left that you wish to have applied given @CaseyCarter's comments, or is this all LWG material?

I don't think I'm likely to do anything else with this PR, so I think it should just be closed at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes to the wording or approach have been requested and not yet applied. lwg Issue must be reviewed by LWG. needs rebase The pull request needs a git rebase to resolve merge conflicts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants