[PATCH] D129951: [clang] teaches Clang the special ADL rules for functions in std::ranges

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.


================
Comment at: clang/lib/Sema/SemaOverload.cpp:9438
   // FIXME: Pass in the explicit template arguments?
   ArgumentDependentLookup(Name, Loc, Args, Fns);
 
----------------
rsmith wrote:
> It would seem preferable to me to do the filtering in `ArgumentDependentLookup` itself rather than here (but I don't feel strongly about it).
Agreed. I tried to do it there, but was having issues getting it working. I'll give it a second go?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:9455-9459
+    // Functions in 'std::ranges' are hidden from ADL per [range.iter.ops]/2 and
+    // [algorithms.requirements]/2.
+    if ((*I)->isInStdRangesNamespace() &&
+        Name.getNameKind() == DeclarationName::NameKind::Identifier)
+      continue;
----------------
rsmith wrote:
> I worry that this might break some stdlib implementation that finds helper functions via ADL in `std::ranges` somehow. Also, it seems desirable to make this opt-in for the stdlib implementation and indeed for end-user functions not in `std::ranges`.
> 
> Have you considered enabling this behavior with an attribute instead of by detecting whether the function is in `std::ranges`?
You're the second person to have suggested this, and I daresay Aaron will too, based on our prior discussion about this. We'd chatted about `__disable_adl` specifically, so that anyone who uses it won't be affected by a silent change from Clang to GCC: they'd instead get a break. I would prefer an attribute too.

My main concern is that other stdlib implementers would object to adding yet another annotation to their function calls (based on my failure to get libc++ to be as aggressive as Microsoft/STL is with `[[nodiscard]]`).


================
Comment at: clang/lib/Sema/SemaOverload.cpp:12837-12841
+  // Functions in 'std::ranges' inhibit ADL per [range.iter.ops]/2 and
+  // [algorithms.requirements]/2.
+  if (!ULE->decls().empty() && ULE->decls_begin()->isInStdRangesNamespace() &&
+      ULE->getName().getNameKind() == DeclarationName::NameKind::Identifier)
+    return;
----------------
rsmith wrote:
> What should happen if a `using` declaration names one of these things? Should we care about the properties of the underlying declaration (eg, whether the target of the using declaration is in this namespace / has the attribute), or about the found decl (eg, whether the `using` declaration itself is in the namespace / has the attribute)?
> 
> Depending on the answer, we may need to check all declarations in the `UnresolvedLookupExpr`, not only the first one. For example, we could have an overload set that contains both a user-declared function and a `using` declaration that names a function in `std::ranges` here.
> When found by unqualified ([basic.lookup.unqual]) name lookup for the postfix-expression in a function call ([expr.call]), they inhibit argument-dependent name lookup.

My interpretation of this is that both using-declarations and -directives are impacted (regardless of what the code says right now). [basic.lookup.unqual] doesn't specifically say anything about using-declarations, but I think [p4](https://eel.is/c++draft/basic.lookup.unqual#4) implies that they're included.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129951/new/

https://reviews.llvm.org/D129951