This is an archive of the discontinued LLVM Phabricator instance.

adds `__disable_adl` attribute
Needs ReviewPublic

Authored by cjdb on Jul 16 2022, 8:23 PM.

Details

Reviewers
aaron.ballman
erichkeane
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

cjdb created this revision.Jul 16 2022, 8:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 8:23 PM
cjdb requested review of this revision.Jul 16 2022, 8:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2022, 8:23 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cjdb updated this revision to Diff 445285.Jul 16 2022, 8:28 PM

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 updated this revision to Diff 445286.Jul 16 2022, 8:33 PM
cjdb edited the summary of this revision. (Show Details)

updates an inaccuracy in the commit message

cjdb added a comment.EditedJul 16 2022, 11:30 PM

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 added a comment.Jul 17 2022, 9:31 AM

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 added a subscriber: rsmith.Jul 17 2022, 10:33 AM
rsmith added inline comments.
clang/lib/Sema/SemaOverload.cpp
9438

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

9455–9459

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?

12837–12841

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 added inline comments.Jul 17 2022, 11:29 AM
clang/lib/Sema/SemaOverload.cpp
9438

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

9455–9459

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

12837–12841

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 updated this revision to Diff 492234.Jan 25 2023, 12:45 PM
cjdb retitled this revision from [clang] teaches Clang the special ADL rules for functions in std::ranges to adds `__disable_adl` attribute.
cjdb edited the summary of this revision. (Show Details)
  • updates patch so that it meets what Aaron and I want, also meets most of Richard's feedback
  • updates commit message
cjdb added inline comments.Jan 25 2023, 12:48 PM
clang/lib/Sema/SemaOverload.cpp
12764–12766

This and below need to be deleted.

shafik added a subscriber: shafik.Jan 25 2023, 1:48 PM
shafik added inline comments.
clang/include/clang/Basic/AttrDocs.td
542 ↗(On Diff #492234)

A lot of these look like unrelated changes.

clang/lib/Parse/ParseDecl.cpp
3788–3789 ↗(On Diff #492234)
clang/lib/Sema/SemaOverload.cpp
6402–6403

Unrelated change?

12835
cjdb updated this revision to Diff 492296.Jan 25 2023, 4:56 PM
cjdb marked 6 inline comments as done.

responds to feedback

clang/include/clang/Basic/AttrDocs.td
542 ↗(On Diff #492234)

I tend to not argue with autoformatters.

rsmith added inline comments.Feb 6 2023, 2:58 PM
clang/include/clang/Basic/Attr.td
4132 ↗(On Diff #492296)

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 ↗(On Diff #492296)

OK!

clang/lib/Sema/SemaDeclAttr.cpp
5756 ↗(On Diff #492296)

Maybe also global operator new / operator delete?

cjdb added inline comments.Feb 6 2023, 3:38 PM
clang/include/clang/Basic/Attr.td
4132 ↗(On Diff #492296)

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 ↗(On Diff #492296)

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

aaron.ballman added reviewers: erichkeane, Restricted Project.Mar 7 2023, 6:47 AM
aaron.ballman added inline comments.
clang/include/clang/Basic/Attr.td
4132 ↗(On Diff #492296)

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
6851 ↗(On Diff #492296)

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

542 ↗(On Diff #492234)

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!

clang/lib/Sema/SemaDeclAttr.cpp
5756 ↗(On Diff #492296)

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

cor3ntin added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
5756 ↗(On Diff #492296)

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

clang/lib/Sema/SemaOverload.cpp
13801–13802

White space only changes

aaron.ballman added inline comments.Mar 7 2023, 7:04 AM
clang/lib/Sema/SemaDeclAttr.cpp
5756 ↗(On Diff #492296)

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 added inline comments.Mar 7 2023, 7:05 AM
clang/lib/Sema/SemaDeclAttr.cpp
5756 ↗(On Diff #492296)

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

philnik added a subscriber: philnik.Mar 8 2023, 7:35 AM

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
9455–9459

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

cjdb added a comment.Mar 8 2023, 11:08 AM

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.

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 added a comment.Mar 8 2023, 2:54 PM

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 added a subscriber: ldionne.Mar 8 2023, 3:00 PM

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.

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 added a comment.Mar 8 2023, 3:43 PM

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.

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.

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.

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!

cjdb updated this revision to Diff 515497.Apr 20 2023, 2:50 PM
cjdb marked 6 inline comments as done.
  • undoes whitespace changes as requested
  • documents feature

I think the only thing left is to address the global new/delete and static member function discussion, which should be quickly implementable!

cjdb added inline comments.Apr 20 2023, 2:58 PM
clang/lib/Sema/SemaDeclAttr.cpp
5756 ↗(On Diff #492296)

Do we have a use-case for this? I'd rather start with global functions and add static member functions if a genuine need arises.

cjdb added a comment.Apr 27 2023, 2:29 PM

Gentle ping :)

cjdb added a comment.May 1 2023, 9:49 AM

Ping (moving my pings from Thursday afternoon to Monday mornings)

Ping (moving my pings from Thursday afternoon to Monday mornings)

Apologies for the delay in responding, but I was actually silent in the hopes that the discussion around proposing to WG21 and how much general use this feature sees would materialize a bit more. https://clang.llvm.org/get_involved.html#criteria has two criteria that I think apply here: "Evidence of a significant user community" and "Representation within the appropriate governing organization". The fact that this could potentially be used by an STL or Abseil and drastically reduce compile times with a more direct approach than function objects suggests that there might be a significant user community that would benefit from this, but if those projects would struggle to adopt this feature because they need to handle older Clang versions or non-Clang compilers, that community may not materialize. Further, because this is being proposed as a keyword (which I think makes sense over an attribute because a correct program with this construct will not necessarily remain correct in its absence due to different lookup results) and it seems to be functionality that would be appropriate to standardize, there should be some collaboration with WG21 if only to hear back "we don't see a reason such an extension would be a problem for us or conflict with existing efforts".

Specific to this review, however, this is missing documentation in LanguageExtensions.rst for the feature, how to use it, etc. as well as release notes. How does this marking fit into the language? How does it impact redeclaration merging (do all declarations have to have the marking, or is it additive like attributes often are?), how does it impact ODR (do declarations have to match across TU boundaries?), is this a declaration specifier or something else (can you do void __disable_adl func() or does it have to be __disable_adl void func();?), that sort of stuff.