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

[contents,fs.req.namespace] Qualify only namespace-scope names LWG2818 #1913

Merged
merged 1 commit into from Nov 2, 2021

Conversation

jensmaurer
Copy link
Member

It does not make sense to attempt to qualify class members
with a nested-name-specifier consisting of namespace-names.

Fixes #1793.

@jensmaurer
Copy link
Member Author

jensmaurer commented Mar 16, 2018

Editorial meeting: What if there are nested namespaces? What if this is ambiguous (different namespaces)?
Simply say "Unqualified names don't perform ADL".
Don't say anything in the filesystem section.

@jensmaurer
Copy link
Member Author

I have rebased the patch, but it does not yet implement the "editorial meeting" outcome. This patch is currently not ready to be merged.

@zygoloid
Copy link
Member

zygoloid commented Oct 8, 2018

I would like to make sure that LWG has a chance to look at this and comment on it before it's applied.

@zygoloid
Copy link
Member

zygoloid commented Oct 8, 2018

We appear to have quite a few places that fail to satisfy this "unless otherwise specified" rule (these are pre-existing errors, but still):

[optional.swap]'s Table 43 says "calls swap(*(*this), *rhs) without saying that ADL is performed
[variant.swap]/2.2 likewise says 'calls swap(...)' without saying that ADL is performed, and similarly in /3 and /4
[queue.defn]'s inline definition of std::queue<T>::swap is:

void swap(queue& q) noexcept(is_nothrow_swappable_v<Container>)
  { using std::swap; swap(c, q.c); }

... which is wrong, and likewise for [priqueue.overview] and [stack.defn].
[alg.swap]/2 likewise says swap(*(first1 + n), *(first2 + n)), and [alg.swap]/6 says swap(*a, *b) with no invocation of ADL.

[container.node.modifiers] likewise uses swap without a "use ADL" proviso, but I think that one is actually correct: we don't want user code interfering with node_handle swaps, but user types can absolutely be associated classes of that lookup

The definition of is_swappable_with deals with this by saying "in an overload-resolution context for swappable values (15.5.3.2)", and looks like the only place we get this right other than the definition of the "swappable" requirements and Swappable concepts.

All this makes me think that perhaps we should move the "swap is found by ADL" rule into the front matter: "Unless otherwise specified, argument-dependent name lookup is only performed within the standard library for operator-function-ids and for the identifier swap." -- and we should change [container.node.modifiers] to explicitly specify that ADL is not performed. Though in the short term, we could instead update the places listed above to say "in an overload-resolution context for swappable values".

@jensmaurer
Copy link
Member Author

Rebased,

@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label Dec 7, 2018
@jensmaurer jensmaurer changed the title [contents,fs.req.namespace] Qualify only namespace-scope names [contents,fs.req.namespace] Qualify only namespace-scope names LWG 2818 Jun 27, 2020
@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label Apr 4, 2021
@jensmaurer
Copy link
Member Author

I've added the front-matter talk about "swap", which results in fewer changes than talking about ADL everywhere "swap" appears.

@jwakely, could you have LWG look at this?

@jensmaurer jensmaurer added the decision-required A decision of the editorial group (or the Project Editor) is required. label Apr 4, 2021
@jensmaurer
Copy link
Member Author

jensmaurer commented Apr 30, 2021

Editorial 2021-04-30: Remove iostreams.tex changes; they are covered by LWG2818.
The lib-intro.tex change needs to wait for LWG2818 to land.

@jensmaurer jensmaurer removed the decision-required A decision of the editorial group (or the Project Editor) is required. label Apr 30, 2021
source/iostreams.tex Outdated Show resolved Hide resolved
source/lib-intro.tex Outdated Show resolved Hide resolved
source/lib-intro.tex Outdated Show resolved Hide resolved
@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label May 24, 2021
@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jun 15, 2021
@jensmaurer jensmaurer changed the title [contents,fs.req.namespace] Qualify only namespace-scope names LWG 2818 [contents,fs.req.namespace] Qualify only namespace-scope names LWG2818 Jun 15, 2021
@jensmaurer jensmaurer removed 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 Jul 4, 2021
@jensmaurer
Copy link
Member Author

The only residual open issue is the special treatment of swap. See the updated diff.

@jensmaurer jensmaurer removed the lwg Issue must be reviewed by LWG. label Jul 5, 2021
@jensmaurer jensmaurer added the decision-required A decision of the editorial group (or the Project Editor) is required. label Jul 20, 2021
@jensmaurer jensmaurer removed the decision-required A decision of the editorial group (or the Project Editor) is required. label Oct 25, 2021
@jensmaurer
Copy link
Member Author

@jwakely, this adds special treatment for swap to avoid extra words everywhere. Please have a look.

Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

make_error_code and make_error_condition also need to be looked up using ADL, but I haven't opened the LWG issue about that yet.

@jensmaurer
Copy link
Member Author

@tkoeppe, this looks ready now.

@jwakely
Copy link
Member

jwakely commented Nov 1, 2021

@tkoeppe tkoeppe merged commit f3ab334 into cplusplus:main Nov 2, 2021
@jensmaurer jensmaurer deleted the b1 branch November 2, 2021 21:48
@jensmaurer
Copy link
Member Author

jensmaurer commented Nov 3, 2021

when the paper standard says swap_ranges, the implementor should write std::swap_ranges in the code

This is not accurate:

namespace N {
  namespace std {
    void f();
  }
}

using namespace N;

namespace std {
  void f();
  void g() { std::f(); }
}
x.cc: In function ‘void std::g()’:
x.cc:12:14: error: reference to ‘std’ is ambiguous
   12 |   void g() { std::f(); }
      |              ^~~
x.cc:3:13: note: candidates are: ‘namespace N::std { }’
    3 |   namespace std {
      |             ^~~
<built-in>: note:                 ‘namespace std { }’

So, the implementer needs to use ::std::function_name() to disambiguate properly.
(If the function is not overloaded or a template, you might also get away with something like (+f)() (convert to a function pointer first, which doesn't use ADL).)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I do not think [fs.req.namespaces] means what it thinks it means LWG2818
6 participants