Message ID | 20230321111056.78121-2-kmatsui@cs.washington.edu |
---|---|
State | New |
Headers | show |
Series | [1/2] c++: implement __add_const built-in trait | expand |
On Tue, 21 Mar 2023, Ken Matsui via Libstdc++ wrote: > /// add_const > +#if __has_builtin(__add_const) > + template<typename _Tp> > + struct add_const > + { using type = __add_const(_Tp); }; > +#else > template<typename _Tp> > struct add_const > { using type = _Tp const; }; > +#endif Is that really better? You asked elsewhere if you should measure for each patch, and I think that at least for such a trivial case, you need to demonstrate that there is a point. The drawbacks are obvious: more code in libstdc++, non-standard, and more builtins in the compiler. Using builtins makes more sense for complicated traits where you can save several instantiations. Now that you have done a couple simple cases to see how it works, I think you should concentrate on the more complicated cases.
On Tue, 21 Mar 2023 at 11:21, Marc Glisse via Libstdc++ < libstdc++@gcc.gnu.org> wrote: > On Tue, 21 Mar 2023, Ken Matsui via Libstdc++ wrote: > > > /// add_const > > +#if __has_builtin(__add_const) > > + template<typename _Tp> > > + struct add_const > > + { using type = __add_const(_Tp); }; > > +#else > > template<typename _Tp> > > struct add_const > > { using type = _Tp const; }; > > +#endif > > Is that really better? You asked elsewhere if you should measure for each > patch, and I think that at least for such a trivial case, you need to > demonstrate that there is a point. The drawbacks are obvious: more code in > libstdc++, non-standard, and more builtins in the compiler. > Right, this one isn't even getting rid of any partial specializations, but it is giving the preprocessor more work to do. Adding the extra built-ins to the compiler makes the compiler (very slightly) bigger and slower, so a real benchmark would require comparing an unpatched gcc (without the new built-in) to a patched gcc and patched libstdc++ sources. > > Using builtins makes more sense for complicated traits where you can save > several instantiations. Now that you have done a couple simple cases to > see how it works, I think you should concentrate on the more complicated > cases. > > -- > Marc Glisse > >
Thank you for your information. Although it matches my intuition, I sent this patch because I was unsure my intuition was correct. As Jonathan pointed out, there appear to be several implementation errors. The benchmark result for this trait is kind of trivial, so I will implement the other traits I want to implement and then come back here. Thank you all for your help. On Tue, Mar 21, 2023 at 4:25 AM Jonathan Wakely <jwakely@redhat.com> wrote: > > > On Tue, 21 Mar 2023 at 11:21, Marc Glisse via Libstdc++ < > libstdc++@gcc.gnu.org> wrote: > >> On Tue, 21 Mar 2023, Ken Matsui via Libstdc++ wrote: >> >> > /// add_const >> > +#if __has_builtin(__add_const) >> > + template<typename _Tp> >> > + struct add_const >> > + { using type = __add_const(_Tp); }; >> > +#else >> > template<typename _Tp> >> > struct add_const >> > { using type = _Tp const; }; >> > +#endif >> >> Is that really better? You asked elsewhere if you should measure for each >> patch, and I think that at least for such a trivial case, you need to >> demonstrate that there is a point. The drawbacks are obvious: more code >> in >> libstdc++, non-standard, and more builtins in the compiler. >> > > Right, this one isn't even getting rid of any partial specializations, but > it is giving the preprocessor more work to do. > > Adding the extra built-ins to the compiler makes the compiler (very > slightly) bigger and slower, so a real benchmark would require comparing an > unpatched gcc (without the new built-in) to a patched gcc and patched > libstdc++ sources. > > > >> >> Using builtins makes more sense for complicated traits where you can save >> several instantiations. Now that you have done a couple simple cases to >> see how it works, I think you should concentrate on the more complicated >> cases. >> >> -- >> Marc Glisse >> >>
diff --git a/libstdc++-v3/include/std/type_traits b/libstdc++-v3/include/std/type_traits index 2bd607a8b8f..1ac75a928c3 100644 --- a/libstdc++-v3/include/std/type_traits +++ b/libstdc++-v3/include/std/type_traits @@ -1560,9 +1560,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION #endif /// add_const +#if __has_builtin(__add_const) + template<typename _Tp> + struct add_const + { using type = __add_const(_Tp); }; +#else template<typename _Tp> struct add_const { using type = _Tp const; }; +#endif /// add_volatile template<typename _Tp>