cjdb added a comment. In D129951#4178949 <https://reviews.llvm.org/D129951#4178949>, @philnik wrote:
> In D129951#4178844 <https://reviews.llvm.org/D129951#4178844>, @cjdb wrote: > >> In D129951#4178154 <https://reviews.llvm.org/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 <https://abseil.io/about/compatibility> 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 <https://godbolt.org/z/13z65EY88> >> 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 >> <https://gist.github.com/cjdb/6ade504f010dc550890a82f3a5c0ea6a>. 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 <https://godbolt.org/z/ncq1qx5Ys> 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129951/new/ https://reviews.llvm.org/D129951 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
