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
Conversation
source/concepts.tex
Outdated
\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}. |
There was a problem hiding this comment.
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}. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
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. |
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,
This seems very worrisome if true! But I don't understand your example. Here's my attempt to reproduce it with [*] — 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 |
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 : |
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:
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
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 |
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? |
There was a problem hiding this 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}. |
There was a problem hiding this comment.
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)}, |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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}. |
There was a problem hiding this comment.
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))}, |
There was a problem hiding this comment.
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))}, |
There was a problem hiding this comment.
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))}, |
There was a problem hiding this comment.
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))}, |
There was a problem hiding this comment.
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))}, |
There was a problem hiding this comment.
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.
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; |
There was a problem hiding this comment.
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.
@Quuxplusone, is there some editorial cleanup/harmonization left that you wish to have applied given @CaseyCarter's comments, or is this all LWG material? |
@Quuxplusone, ping? |
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. |
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:
(For example,
ranges::swap
doesn't need a poison pill becausestd::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.