adds `__disable_adl` attribute

[cjdb]

Summary

Certain library functions need to disable ADL. For example, the functions
in [range.iter.ops]/2 and [algorithms.requirements]/2 require functions
declared in namespaces 'std::ranges' aren't found by ADL, and that they
suppress ADL when called in an unqualified context (e.g. by way of a
_using-directive_). Conversations with developers on the #include <C++>
Discord channel identifies that there's a desire for this outside the
ranges library.

Libraries have been implementing these functions as function objects
with varying rules (e.g. libc++ and Microsoft/STL both try their best to
make the function objects appear as standard library function templates,
while libstdc++ makes them plain function objects). Having a large number
of types typically has a negative impact on both compile-times and progam
size.

This commit has seen compile times halved when the __disable_adl
attribute is applied instead of opting to use a function object.

[cjdb]

moves test from Sema to SemaCXX

I Noticed that SemaCXX is only a test directory, and that there's no corresponding SemaCXX in either include or lib.

[cjdb]

updates an inaccuracy in the commit message

[cjdb]

Looking at the output from Clang 14, I'm observing that a binary with 178 function templates is 13% the size of the one with 89 function objects. When only one function object is used vs all 178 function templates, the functions still win out, with the binary being 80% the size.

The AST also becomes substantially larger.

I ran time clang++-13 -c file.cpp -std=c++20 -Oz -DNDEBUG locally, a hundred times, and got the following results:

# Function objects
real mean:	0.07447
real median:	0.074
real stddev:	0.002267268191
	
sys mean:	0.01664
sys median:	0.016
sys stddev:	0.005569614534
	
user mean:	0.05785
user median:	0.057
user stddev:	0.005848896987

# Function templates
real mean:	0.06336
real median:	0.063
real stddev:	0.002076905235
	
sys mean:	0.01701
sys median:	0.017
sys stddev:	0.005545942188
	
user mean:	0.04645
user median:	0.047
user stddev:	0.005678908346

[cjdb]

In D129951#3657923, @cjdb wrote:

Looking at the output from Clang 14, I'm observing that a binary with 178 function templates is 13% the size of the one with 89 function objects. When only one function object is used vs all 178 function templates, the functions still win out, with the binary being 80% the size.

I wrote this quite late at night and confused total lines of assembly with program size. That's not accurate at all.

There doesn't seem to be a difference between the two when the optimiser is enabled, but with:

• -O0: there's a binary size difference of 24kB.
• -Og -g: there's a binary size difference of 32kB.
• -g: there's a binary size difference of 59kB.

[rsmith]

clang/lib/Sema/SemaOverload.cpp

9584

It would seem preferable to me to do the filtering in ArgumentDependentLookup itself rather than here (but I don't feel strongly about it).

9601–9605

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?

13012–13016

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.

[cjdb]

clang/lib/Sema/SemaOverload.cpp

9584

Agreed. I tried to do it there, but was having issues getting it working. I'll give it a second go?

9601–9605

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]]).

13012–13016

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 implies that they're included.

[cjdb]

• updates patch so that it meets what Aaron and I want, also meets most of Richard's feedback
• updates commit message

[cjdb]

clang/lib/Sema/SemaOverload.cpp

12931–12933

This and below need to be deleted.

[shafik]

clang/include/clang/Basic/AttrDocs.td

542

A lot of these look like unrelated changes.

clang/lib/Parse/ParseDecl.cpp

3788–3789

clang/lib/Sema/SemaOverload.cpp

6514

Unrelated change?

13010

[cjdb]

responds to feedback

clang/include/clang/Basic/AttrDocs.td

542

I tend to not argue with autoformatters.

[cjdb]

Ping @aaron.ballman @rsmith

[cjdb]

Ping @aaron.ballman

[rsmith]

clang/include/clang/Basic/Attr.td

4132

Has this syntax been discussed already? If not, why did you choose keyword syntax instead of a normal attribute?

clang/include/clang/Basic/AttrDocs.td

6851

OK!

clang/lib/Sema/SemaDeclAttr.cpp

5756

Maybe also global operator new / operator delete?

[cjdb]

clang/include/clang/Basic/Attr.td

4132

Aaron and I discussed this offline. I'd originally intended to do [[clang::disable_adl]], but Aaron pointed out that this would be problematic for all involved if someone uses a library in GCC, since the feature is implicitly turned off. If we use this syntax, then it forces libraries to acknowledge (and possibly document) that ADL is restricted on Clang, but potentially not GCC.

I will add a note to the documentation that I'm promising.

clang/lib/Sema/SemaDeclAttr.cpp

5756

How does ADL even work in with the global namespace? Should it be invalid to apply here?

[aaron.ballman]

clang/include/clang/Basic/Attr.td

4132

Yup! The basic thinking I had is: if this attribute is ignored by an implementation, it's very possible that the user's code may change behavior -- if they're unlucky, it's a silent behavior change. However, if it's a keyword, then there's a syntax error and no chance for a silent behavior change.

If you think we need a more broad discussion of the syntax, that'd be fine by me -- it's possible there's a policy question hiding in here about when we want keywords and when we are fine with regular attribute syntax (type attributes are another source of these kinds of questions).

clang/include/clang/Basic/AttrDocs.td

542

We still ask that you don't commit unrelated changes (this was stripping trailing whitespace, which is often an editor setting) -- it makes git blame harder than it needs to be. Feel free to commit the whitespace fixes before/after the patch if you'd like, though!

6851

Btw, as a reviewer: thank you for this. I appreciate clear markings of "we're not done yet!" in the source.

clang/lib/Sema/SemaDeclAttr.cpp

5756

Should this be isCXXInstanceMember() instead? Or do we intend to disallow this on static member functions?

[cor3ntin]

clang/lib/Sema/SemaDeclAttr.cpp

5756

Static member functions would be qualified, and not involve ADL. This seems correct.

clang/lib/Sema/SemaOverload.cpp

13972–13973

White space only changes

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

5756

Hmmm, I think they're *normally* qualified, but if you call the static function from within a member function, it doesn't have to be qualified.

[cor3ntin]

clang/lib/Sema/SemaDeclAttr.cpp

5756

nvm, they can be found by adl from within member functions.

[philnik]

I don't think libc++ can adopt this without having to essentially duplicate our code, since GCC doesn't support __disable_adl (and AFAICT there is no coordination between GCC and Clang to add it to both). Have you tested what impact making the members static has? Both clang and GCC already support this as an extension back to C++11: https://godbolt.org/z/drE5v8nYo. Maybe it would make more sense to add an attribute [[clang::cpo]] instead to tell clang that the class should just be treated as an overload set? Make it requirements that the class is empty, there are no non-static member functions and the class is declared final and you should get any benefits without the major drawback of basically no portability. It's of course possible that I'm missing something major, but I think that general way would be a lot more acceptable. Any thoughts?

clang/lib/Sema/SemaOverload.cpp

9601–9605

FWIW we are getting more aggressive with adding [[nodiscard]]. We have extensions enabled by default and are (slowly) adding it to more places.

[cjdb]

In D129951#4178154, @philnik wrote:

I don't think libc++ can adopt this without having to essentially duplicate our code, since GCC doesn't support __disable_adl (and AFAICT there is no coordination between GCC and Clang to add it to both).

I haven't had a lot of time to drive this in Clang, let alone GCC. Even if libc++ can't ultimately use it (which would be sad), there are other libraries that can. For example, Abseil has a similar attitude towards functions as Niebloids, and could wrap it behind a macro.

Have you tested what impact making the members static has? Both clang and GCC already support this as an extension back to C++11: https://godbolt.org/z/drE5v8nYo.

A quick change to the original benchmark shows the AST for static operator() being substantially larger than a function template with ADL being disabled. I haven't properly benchmarked build time impact, but here's a quick one. The averages are below:

__disable_adl

real  0.1164
user  0.0706
sys   0.0488

static operator()

real  0.1272
user  0.081
sys   0.0488

It is worth acknowledging that the assembly output is now much closer with optimised flags (1.63x larger as opposed to 7.56x larger), but 1.26x larger with -g (this is down from 1.66x as non-static).

Maybe it would make more sense to add an attribute [[clang::cpo]] instead to tell clang that the class should just be treated as an overload set? Make it requirements that the class is empty, there are no non-static member functions and the class is declared final and you should get any benefits without the major drawback of basically no portability. It's of course possible that I'm missing something major, but I think that general way would be a lot more acceptable. Any thoughts?

CPOs and Niebloids are different things (and __disable_adl is for Niebloids, not CPOs), so any such attribute would need a different name. Having said that, a struct that hasn't has no base and is final only slightly improves the AST size with respect to the improvement by using an actual overload set. Finally, there would still be a portability issue because even if [[clang::niebloid]] works on Clang, there would still need to be coordination for it to work on GCC; otherwise GCC w/ libc++ mode would have copyable Niebloids; something that the original libc++ design worked hard to ensure wasn't possible so that a feature like this could exist.

It is again worth acknowledging that the assembly output in an optimised build would have parity, but a build using -O0 -g will still be ~1.26x larger.

[philnik]

In D129951#4178844, @cjdb wrote:

In D129951#4178154, @philnik wrote:

I don't think libc++ can adopt this without having to essentially duplicate our code, since GCC doesn't support __disable_adl (and AFAICT there is no coordination between GCC and Clang to add it to both).

I haven't had a lot of time to drive this in Clang, let alone GCC. Even if libc++ can't ultimately use it (which would be sad), there are other libraries that can. For example, Abseil has a similar attitude towards functions as Niebloids, and could wrap it behind a macro.

Abseil has the same support problem though AFAICT. In fact, most open source libraries don't just support clang.

Have you tested what impact making the members static has? Both clang and GCC already support this as an extension back to C++11: https://godbolt.org/z/drE5v8nYo.

A quick change to the original benchmark shows the AST for static operator() being substantially larger than a function template with ADL being disabled. I haven't properly benchmarked build time impact, but here's a quick one. The averages are below:

__disable_adl

real  0.1164
user  0.0706
sys   0.0488

static operator()

real  0.1272
user  0.081
sys   0.0488

It is worth acknowledging that the assembly output is now much closer with optimised flags (1.63x larger as opposed to 7.56x larger), but 1.26x larger with -g (this is down from 1.66x as non-static).

Couldn't that be overcome with some optimizations for Niebloids?

Maybe it would make more sense to add an attribute [[clang::cpo]] instead to tell clang that the class should just be treated as an overload set? Make it requirements that the class is empty, there are no non-static member functions and the class is declared final and you should get any benefits without the major drawback of basically no portability. It's of course possible that I'm missing something major, but I think that general way would be a lot more acceptable. Any thoughts?

CPOs and Niebloids are different things (and __disable_adl is for Niebloids, not CPOs), so any such attribute would need a different name.

Yes. Sorry for the conflation.

Having said that, a struct that hasn't has no base and is final only slightly improves the AST size with respect to the improvement by using an actual overload set. Finally, there would still be a portability issue because even if [[clang::niebloid]] works on Clang, there would still need to be coordination for it to work on GCC; otherwise GCC w/ libc++ mode would have copyable Niebloids; something that the original libc++ design worked hard to ensure wasn't possible so that a feature like this could exist.

I don't know about the original design, but at least the algorithms are copyable. I wouldn't be too concerned if that was different between clang and GCC, it's at least conforming in both cases. Regarding AST size, I don't know how representative LoC in the dump are, but shouldn't it be possible to overcome memory usage by modeling Niebloids in a different way than normal classes?

It is again worth acknowledging that the assembly output in an optimised build would have parity, but a build using -O0 -g will still be ~1.26x larger.

[cjdb]

In D129951#4178949, @philnik wrote:

In D129951#4178844, @cjdb wrote:

In D129951#4178154, @philnik wrote:

I don't think libc++ can adopt this without having to essentially duplicate our code, since GCC doesn't support __disable_adl (and AFAICT there is no coordination between GCC and Clang to add it to both).

I haven't had a lot of time to drive this in Clang, let alone GCC. Even if libc++ can't ultimately use it (which would be sad), there are other libraries that can. For example, Abseil has a similar attitude towards functions as Niebloids, and could wrap it behind a macro.

Abseil has the same support problem though AFAICT. In fact, most open source libraries don't just support clang.

Abseil already doesn't support calling unqualified functions, so this is a QoI improvement, rather than a correctness one like std::ranges::next. There's no portability issue because it's just a compatibility rule of using Abseil that can now get checked at compile time.

#if defined(__clang__) and __clang_major__ > 18
#  define ABSL_DISABLE_ADL __disable_adl
#else
#  define ABSL_DISABLE_ADL
#endif

namespace absl {
  template<class T>
  ABSL_DISABLE_ADL void func(T);
}

The reason Abseil can get away with it while libc++ cannot is because Abseil doesn't need to adhere to a standard saying "ADL shall not be involved", but rather has the attitude "if you want to painlessly use our library, always qualify your calls to our functions".

Have you tested what impact making the members static has? Both clang and GCC already support this as an extension back to C++11: https://godbolt.org/z/drE5v8nYo.

A quick change to the original benchmark shows the AST for static operator() being substantially larger than a function template with ADL being disabled. I haven't properly benchmarked build time impact, but here's a quick one. The averages are below:

__disable_adl

real  0.1164
user  0.0706
sys   0.0488

static operator()

real  0.1272
user  0.081
sys   0.0488

It is worth acknowledging that the assembly output is now much closer with optimised flags (1.63x larger as opposed to 7.56x larger), but 1.26x larger with -g (this is down from 1.66x as non-static).

Couldn't that be overcome with some optimizations for Niebloids?

Potentially, but not doing work is a better situation to be in than doing work and then having to do more work to evaluate whether or not to keep it.

Maybe it would make more sense to add an attribute [[clang::cpo]] instead to tell clang that the class should just be treated as an overload set? Make it requirements that the class is empty, there are no non-static member functions and the class is declared final and you should get any benefits without the major drawback of basically no portability. It's of course possible that I'm missing something major, but I think that general way would be a lot more acceptable. Any thoughts?

CPOs and Niebloids are different things (and __disable_adl is for Niebloids, not CPOs), so any such attribute would need a different name.

Yes. Sorry for the conflation.

Having said that, a struct that hasn't has no base and is final only slightly improves the AST size with respect to the improvement by using an actual overload set. Finally, there would still be a portability issue because even if [[clang::niebloid]] works on Clang, there would still need to be coordination for it to work on GCC; otherwise GCC w/ libc++ mode would have copyable Niebloids; something that the original libc++ design worked hard to ensure wasn't possible so that a feature like this could exist.

I don't know about the original design, but at least the algorithms are copyable. I wouldn't be too concerned if that was different between clang and GCC, it's at least conforming in both cases.

I'm deeply disappointed that libc++ moved away from using __function_like: that was an important part of preventing niebloid misuse. It isn't conforming to treat niebloids as function objects, which is what __function_like prevented (I just checked std::ranges::next and it seems that __function_like was completely removed).

Regarding AST size, I don't know how representative LoC in the dump are, but shouldn't it be possible to overcome memory usage by modeling Niebloids in a different way than normal classes?

Each line of the AST dump should represent a branch in the tree.

Notice that even if [we delete the entirety of main](https://godbolt.org/z/3zKza3T5P), you'll notice that the AST is still double the size when using structs over functions (since libc++ is no longer using __function_like, I've removed that from the linked comparison, but it clocked in at ~+1000 lines of AST). That's because the structs still need to be a part of the AST regardless of use.

shouldn't it be possible to overcome memory usage by modeling Niebloids in a different way than normal classes?

I think this would require a significant overhaul of how Clang processes structs.

It is again worth acknowledging that the assembly output in an optimised build would have parity, but a build using -O0 -g will still be ~1.26x larger.

[ldionne]

This is a really neat attribute! FWIW, I think we would most likely have used it in <ranges> if it were available back then. Assuming that GCC implemented it, I think we could consider changing to use this attribute. However:

1. This would technically be an ABI break, but this is probably not an issue in practice since I doubt this would bite any user, ever. We'd still have to think about it more seriously.
2. This would definitely be a source break for users, since they wouldn't be able to pass niebloids around as function arguments anymore. This one, I suspect, might be a deal breaker depending on how many users have started depending on that. I suspect this means the ship might have sailed for libc++ to use it in existing code.

FWIW, my main thought is that I would like to see this proposed as a WG21 proposal. This might end up an attribute or something else, I'm not sure. But it would address the issue of not all implementations providing the functionality. IMO this patch and the data gathered in it would be a great motivation for WG21 to do something (I also know there are other proposals in this domain).

Note: For history, we stopped making niebloids non-copyable in D116570. We originally did, but then that created divergence with libstdc++ and meant that we had to distinguish between CPOs and Niebloids in a way that seemed a bit arbitrary, so instead we went for a straightforward implementation. It is technically not as strict as it could be, though.

[ldionne]

In D129951#4179467, @cjdb wrote:

I'm deeply disappointed that libc++ moved away from using __function_like: that was an important part of preventing niebloid misuse. It isn't conforming to treat niebloids as function objects, which is what __function_like prevented (I just checked std::ranges::next and it seems that __function_like was completely removed).

Oops, it seems like my reply got into a race condition with yours. I am curious though, why is it non-conforming to treat niebloids as function objects? It is certainly more lax than required by the Standard, however we are allowed to make those copyable, aren't we? Does the Standard say that we *have* to make them non-usable as objects? That would change things for sure (not with this patch, but we'd probably want to try re-introducing __function_like, and we'd file a bug report against libstdc++ to do the same).

[cjdb]

In D129951#4179481, @ldionne wrote:

In D129951#4179467, @cjdb wrote:

I'm deeply disappointed that libc++ moved away from using __function_like: that was an important part of preventing niebloid misuse. It isn't conforming to treat niebloids as function objects, which is what __function_like prevented (I just checked std::ranges::next and it seems that __function_like was completely removed).

Oops, it seems like my reply got into a race condition with yours. I am curious though, why is it non-conforming to treat niebloids as function objects? It is certainly more lax than required by the Standard, however we are allowed to make those copyable, aren't we? Does the Standard say that we *have* to make them non-usable as objects? That would change things for sure (not with this patch, but we'd probably want to try re-introducing __function_like, and we'd file a bug report against libstdc++ to do the same).

"isn't conforming" is a bit too harsh in retrospect (this applies to every conversation I've brought this up in, although I meant that it allows users to write "non-conforming code"). A more accurate way to put this is: it is conforming on a per-implementation basis, but is non-portable due to namespace.std/p6. For example, a user who wants to use Microsoft/STL won't ever be able to take advantage of using std::ranges::next as a higher-order function (and could potentially have migration issues if they're porting to Windows), because their niebloids are derived from _Not_quite_object. _Not_quite_object's predecessor in cmcstl2 was the inspiration for __function_like.

As for a refresher on why this is the reason, it's because the functions in std::ranges should be functions, but we lack the technology to make them functions.

In D129951#4179474, @ldionne wrote:

This is a really neat attribute! FWIW, I think we would most likely have used it in <ranges> if it were available back then. Assuming that GCC implemented it, I think we could consider changing to use this attribute. However:

Thanks, glad you like it :-)
(I think the "however" bit got addressed above.)

FWIW, my main thought is that I would like to see this proposed as a WG21 proposal. This might end up an attribute or something else, I'm not sure. But it would address the issue of not all implementations providing the functionality. IMO this patch and the data gathered in it would be a great motivation for WG21 to do something (I also know there are other proposals in this domain).

I don't attend WG21 anymore, but could probably work with someone who is motivated enough to see it from Evolution through plenary. My preference is a specifier, rather than an attribute. For better, or for worse, most attributes are ignorable; this is certainly a property that should never be ignored.

[cor3ntin]

In D129951#4179596, @cjdb wrote:

In D129951#4179481, @ldionne wrote:

In D129951#4179467, @cjdb wrote:

I'm deeply disappointed that libc++ moved away from using __function_like: that was an important part of preventing niebloid misuse. It isn't conforming to treat niebloids as function objects, which is what __function_like prevented (I just checked std::ranges::next and it seems that __function_like was completely removed).

Oops, it seems like my reply got into a race condition with yours. I am curious though, why is it non-conforming to treat niebloids as function objects? It is certainly more lax than required by the Standard, however we are allowed to make those copyable, aren't we? Does the Standard say that we *have* to make them non-usable as objects? That would change things for sure (not with this patch, but we'd probably want to try re-introducing __function_like, and we'd file a bug report against libstdc++ to do the same).

"isn't conforming" is a bit too harsh in retrospect (this applies to every conversation I've brought this up in, although I meant that it allows users to write "non-conforming code"). A more accurate way to put this is: it is conforming on a per-implementation basis, but is non-portable due to namespace.std/p6. For example, a user who wants to use Microsoft/STL won't ever be able to take advantage of using std::ranges::next as a higher-order function (and could potentially have migration issues if they're porting to Windows), because their niebloids are derived from _Not_quite_object. _Not_quite_object's predecessor in cmcstl2 was the inspiration for __function_like.

As for a refresher on why this is the reason, it's because the functions in std::ranges should be functions, but we lack the technology to make them functions.

In D129951#4179474, @ldionne wrote:

This is a really neat attribute! FWIW, I think we would most likely have used it in <ranges> if it were available back then. Assuming that GCC implemented it, I think we could consider changing to use this attribute. However:

Thanks, glad you like it :-)
(I think the "however" bit got addressed above.)

FWIW, my main thought is that I would like to see this proposed as a WG21 proposal. This might end up an attribute or something else, I'm not sure. But it would address the issue of not all implementations providing the functionality. IMO this patch and the data gathered in it would be a great motivation for WG21 to do something (I also know there are other proposals in this domain).

I don't attend WG21 anymore, but could probably work with someone who is motivated enough to see it from Evolution through plenary. My preference is a specifier, rather than an attribute. For better, or for worse, most attributes are ignorable; this is certainly a property that should never be ignored.

@lewissbaker is the person you want to chat with!