| Message ID | 20250721104622.1550908-4-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: > Add `cxx20.h` that will contain (possibly limited) C++17 implementations > of some C++20 standard library features. Our span.h implementation qualifies in this category as well. As I see it 1) We move span.h in /details/ 2) We move cxx20.h in include/libcamera/base What's your take ? > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/base/details/cxx20.h | 12 ++++++++++++ > include/libcamera/base/meson.build | 7 +++++++ > 2 files changed, 19 insertions(+) > create mode 100644 include/libcamera/base/details/cxx20.h > > diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h > new file mode 100644 > index 000000000..0d6173d1b > --- /dev/null > +++ b/include/libcamera/base/details/cxx20.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2025, Ideas on Board Oy > + * > + * C++20 polyfills > + */ > + > +#pragma once > + > +namespace libcamera::details::cxx20 { > + > +} /* namespace libcamera::details::cxx20 */ > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build > index f28ae4d42..9836daff9 100644 > --- a/include/libcamera/base/meson.build > +++ b/include/libcamera/base/meson.build > @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ > 'utils.h', > ]) > > +libcamera_base_details_headers = files([ > + 'details/cxx20.h', > +]) > + > libcamera_base_headers = [ > libcamera_base_public_headers, > libcamera_base_private_headers, > + libcamera_base_details_headers, > ] > > install_headers(libcamera_base_public_headers, > subdir : libcamera_base_include_dir) > +install_headers(libcamera_base_details_headers, > + subdir : libcamera_base_include_dir / 'details') > -- > 2.50.1 >
On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: > Hi Barnabás > > On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: > > Add `cxx20.h` that will contain (possibly limited) C++17 implementations > > of some C++20 standard library features. > > Our span.h implementation qualifies in this category as well. > > As I see it > 1) We move span.h in /details/ > 2) We move cxx20.h in include/libcamera/base > > What's your take ? The issue with our Span class is that it's part of the public API, while I understand libcamera::details::cxx20 to be meant for internal usage. Although... > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > --- > > include/libcamera/base/details/cxx20.h | 12 ++++++++++++ > > include/libcamera/base/meson.build | 7 +++++++ > > 2 files changed, 19 insertions(+) > > create mode 100644 include/libcamera/base/details/cxx20.h > > > > diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h > > new file mode 100644 > > index 000000000..0d6173d1b > > --- /dev/null > > +++ b/include/libcamera/base/details/cxx20.h > > @@ -0,0 +1,12 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2025, Ideas on Board Oy > > + * > > + * C++20 polyfills > > + */ > > + > > +#pragma once > > + > > +namespace libcamera::details::cxx20 { > > + > > +} /* namespace libcamera::details::cxx20 */ > > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build > > index f28ae4d42..9836daff9 100644 > > --- a/include/libcamera/base/meson.build > > +++ b/include/libcamera/base/meson.build > > @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ > > 'utils.h', > > ]) > > > > +libcamera_base_details_headers = files([ > > + 'details/cxx20.h', > > +]) > > + > > libcamera_base_headers = [ > > libcamera_base_public_headers, > > libcamera_base_private_headers, > > + libcamera_base_details_headers, > > ] > > > > install_headers(libcamera_base_public_headers, > > subdir : libcamera_base_include_dir) > > +install_headers(libcamera_base_details_headers, > > + subdir : libcamera_base_include_dir / 'details') ... this seems to indicate the contrary ? Barnabás, could you please explain what your plan is ? If we want to use this in the public API, I think we should shorten the namespace for public classes, possibly to libcamera::cxx20. If it's purely internal, we can't move the span class here, and the headers should probably not be installed.
Hi 2025. 07. 27. 15:30 keltezéssel, Laurent Pinchart írta: > On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: >> Hi Barnabás >> >> On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: >>> Add `cxx20.h` that will contain (possibly limited) C++17 implementations >>> of some C++20 standard library features. >> >> Our span.h implementation qualifies in this category as well. >> >> As I see it >> 1) We move span.h in /details/ >> 2) We move cxx20.h in include/libcamera/base >> >> What's your take ? > > The issue with our Span class is that it's part of the public API, while > I understand libcamera::details::cxx20 to be meant for internal usage. > Although... > >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>> --- >>> include/libcamera/base/details/cxx20.h | 12 ++++++++++++ >>> include/libcamera/base/meson.build | 7 +++++++ >>> 2 files changed, 19 insertions(+) >>> create mode 100644 include/libcamera/base/details/cxx20.h >>> >>> diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h >>> new file mode 100644 >>> index 000000000..0d6173d1b >>> --- /dev/null >>> +++ b/include/libcamera/base/details/cxx20.h >>> @@ -0,0 +1,12 @@ >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>> +/* >>> + * Copyright (C) 2025, Ideas on Board Oy >>> + * >>> + * C++20 polyfills >>> + */ >>> + >>> +#pragma once >>> + >>> +namespace libcamera::details::cxx20 { >>> + >>> +} /* namespace libcamera::details::cxx20 */ >>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build >>> index f28ae4d42..9836daff9 100644 >>> --- a/include/libcamera/base/meson.build >>> +++ b/include/libcamera/base/meson.build >>> @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ >>> 'utils.h', >>> ]) >>> >>> +libcamera_base_details_headers = files([ >>> + 'details/cxx20.h', >>> +]) >>> + >>> libcamera_base_headers = [ >>> libcamera_base_public_headers, >>> libcamera_base_private_headers, >>> + libcamera_base_details_headers, >>> ] >>> >>> install_headers(libcamera_base_public_headers, >>> subdir : libcamera_base_include_dir) >>> +install_headers(libcamera_base_details_headers, >>> + subdir : libcamera_base_include_dir / 'details') > > ... this seems to indicate the contrary ? Barnabás, could you please > explain what your plan is ? > > If we want to use this in the public API, I think we should shorten the > namespace for public classes, possibly to libcamera::cxx20. If it's > purely internal, we can't move the span class here, and the headers > should probably not be installed. > The plan is that these are not part of the public API, merely implementation details that need to be available in public header files, so that they can be used from other public libcamera header files. Applications are not supposed to use them. Regards, Barnabás Pőcze
On Sun, Jul 27, 2025 at 07:15:56PM +0200, Barnabás Pőcze wrote: > 2025. 07. 27. 15:30 keltezéssel, Laurent Pinchart írta: > > On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: > >> On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: > >>> Add `cxx20.h` that will contain (possibly limited) C++17 implementations > >>> of some C++20 standard library features. > >> > >> Our span.h implementation qualifies in this category as well. > >> > >> As I see it > >> 1) We move span.h in /details/ > >> 2) We move cxx20.h in include/libcamera/base > >> > >> What's your take ? > > > > The issue with our Span class is that it's part of the public API, while > > I understand libcamera::details::cxx20 to be meant for internal usage. > > Although... > > > >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>> --- > >>> include/libcamera/base/details/cxx20.h | 12 ++++++++++++ > >>> include/libcamera/base/meson.build | 7 +++++++ > >>> 2 files changed, 19 insertions(+) > >>> create mode 100644 include/libcamera/base/details/cxx20.h > >>> > >>> diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h > >>> new file mode 100644 > >>> index 000000000..0d6173d1b > >>> --- /dev/null > >>> +++ b/include/libcamera/base/details/cxx20.h > >>> @@ -0,0 +1,12 @@ > >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>> +/* > >>> + * Copyright (C) 2025, Ideas on Board Oy > >>> + * > >>> + * C++20 polyfills > >>> + */ > >>> + > >>> +#pragma once > >>> + > >>> +namespace libcamera::details::cxx20 { > >>> + > >>> +} /* namespace libcamera::details::cxx20 */ > >>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build > >>> index f28ae4d42..9836daff9 100644 > >>> --- a/include/libcamera/base/meson.build > >>> +++ b/include/libcamera/base/meson.build > >>> @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ > >>> 'utils.h', > >>> ]) > >>> > >>> +libcamera_base_details_headers = files([ > >>> + 'details/cxx20.h', > >>> +]) > >>> + > >>> libcamera_base_headers = [ > >>> libcamera_base_public_headers, > >>> libcamera_base_private_headers, > >>> + libcamera_base_details_headers, > >>> ] > >>> > >>> install_headers(libcamera_base_public_headers, > >>> subdir : libcamera_base_include_dir) > >>> +install_headers(libcamera_base_details_headers, > >>> + subdir : libcamera_base_include_dir / 'details') > > > > ... this seems to indicate the contrary ? Barnabás, could you please > > explain what your plan is ? > > > > If we want to use this in the public API, I think we should shorten the > > namespace for public classes, possibly to libcamera::cxx20. If it's > > purely internal, we can't move the span class here, and the headers > > should probably not be installed. > > The plan is that these are not part of the public API, merely implementation > details that need to be available in public header files, so that they can > be used from other public libcamera header files. Applications are not supposed > to use them. Then unless I'm mistaken you don't need to install those headers. I also wouldn't put this in a "details" namespace, libcamera::cxx20::* should be fine.
2025. 07. 27. 20:37 keltezéssel, Laurent Pinchart írta: > On Sun, Jul 27, 2025 at 07:15:56PM +0200, Barnabás Pőcze wrote: >> 2025. 07. 27. 15:30 keltezéssel, Laurent Pinchart írta: >>> On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: >>>> On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: >>>>> Add `cxx20.h` that will contain (possibly limited) C++17 implementations >>>>> of some C++20 standard library features. >>>> >>>> Our span.h implementation qualifies in this category as well. >>>> >>>> As I see it >>>> 1) We move span.h in /details/ >>>> 2) We move cxx20.h in include/libcamera/base >>>> >>>> What's your take ? >>> >>> The issue with our Span class is that it's part of the public API, while >>> I understand libcamera::details::cxx20 to be meant for internal usage. >>> Although... >>> >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>> --- >>>>> include/libcamera/base/details/cxx20.h | 12 ++++++++++++ >>>>> include/libcamera/base/meson.build | 7 +++++++ >>>>> 2 files changed, 19 insertions(+) >>>>> create mode 100644 include/libcamera/base/details/cxx20.h >>>>> >>>>> diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h >>>>> new file mode 100644 >>>>> index 000000000..0d6173d1b >>>>> --- /dev/null >>>>> +++ b/include/libcamera/base/details/cxx20.h >>>>> @@ -0,0 +1,12 @@ >>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>>> +/* >>>>> + * Copyright (C) 2025, Ideas on Board Oy >>>>> + * >>>>> + * C++20 polyfills >>>>> + */ >>>>> + >>>>> +#pragma once >>>>> + >>>>> +namespace libcamera::details::cxx20 { >>>>> + >>>>> +} /* namespace libcamera::details::cxx20 */ >>>>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build >>>>> index f28ae4d42..9836daff9 100644 >>>>> --- a/include/libcamera/base/meson.build >>>>> +++ b/include/libcamera/base/meson.build >>>>> @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ >>>>> 'utils.h', >>>>> ]) >>>>> >>>>> +libcamera_base_details_headers = files([ >>>>> + 'details/cxx20.h', >>>>> +]) >>>>> + >>>>> libcamera_base_headers = [ >>>>> libcamera_base_public_headers, >>>>> libcamera_base_private_headers, >>>>> + libcamera_base_details_headers, >>>>> ] >>>>> >>>>> install_headers(libcamera_base_public_headers, >>>>> subdir : libcamera_base_include_dir) >>>>> +install_headers(libcamera_base_details_headers, >>>>> + subdir : libcamera_base_include_dir / 'details') >>> >>> ... this seems to indicate the contrary ? Barnabás, could you please >>> explain what your plan is ? >>> >>> If we want to use this in the public API, I think we should shorten the >>> namespace for public classes, possibly to libcamera::cxx20. If it's >>> purely internal, we can't move the span class here, and the headers >>> should probably not be installed. >> >> The plan is that these are not part of the public API, merely implementation >> details that need to be available in public header files, so that they can >> be used from other public libcamera header files. Applications are not supposed >> to use them. > > Then unless I'm mistaken you don't need to install those headers. I also > wouldn't put this in a "details" namespace, libcamera::cxx20::* should > be fine. > They need to be installed because the plan is that public libcamera headers files will include them.
On Sun, Jul 27, 2025 at 08:44:31PM +0200, Barnabás Pőcze wrote: > 2025. 07. 27. 20:37 keltezéssel, Laurent Pinchart írta: > > On Sun, Jul 27, 2025 at 07:15:56PM +0200, Barnabás Pőcze wrote: > >> 2025. 07. 27. 15:30 keltezéssel, Laurent Pinchart írta: > >>> On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: > >>>> On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: > >>>>> Add `cxx20.h` that will contain (possibly limited) C++17 implementations > >>>>> of some C++20 standard library features. > >>>> > >>>> Our span.h implementation qualifies in this category as well. > >>>> > >>>> As I see it > >>>> 1) We move span.h in /details/ > >>>> 2) We move cxx20.h in include/libcamera/base > >>>> > >>>> What's your take ? > >>> > >>> The issue with our Span class is that it's part of the public API, while > >>> I understand libcamera::details::cxx20 to be meant for internal usage. > >>> Although... > >>> > >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>>> --- > >>>>> include/libcamera/base/details/cxx20.h | 12 ++++++++++++ > >>>>> include/libcamera/base/meson.build | 7 +++++++ > >>>>> 2 files changed, 19 insertions(+) > >>>>> create mode 100644 include/libcamera/base/details/cxx20.h > >>>>> > >>>>> diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h > >>>>> new file mode 100644 > >>>>> index 000000000..0d6173d1b > >>>>> --- /dev/null > >>>>> +++ b/include/libcamera/base/details/cxx20.h > >>>>> @@ -0,0 +1,12 @@ > >>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>>>> +/* > >>>>> + * Copyright (C) 2025, Ideas on Board Oy > >>>>> + * > >>>>> + * C++20 polyfills > >>>>> + */ > >>>>> + > >>>>> +#pragma once > >>>>> + > >>>>> +namespace libcamera::details::cxx20 { > >>>>> + > >>>>> +} /* namespace libcamera::details::cxx20 */ > >>>>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build > >>>>> index f28ae4d42..9836daff9 100644 > >>>>> --- a/include/libcamera/base/meson.build > >>>>> +++ b/include/libcamera/base/meson.build > >>>>> @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ > >>>>> 'utils.h', > >>>>> ]) > >>>>> > >>>>> +libcamera_base_details_headers = files([ > >>>>> + 'details/cxx20.h', > >>>>> +]) > >>>>> + > >>>>> libcamera_base_headers = [ > >>>>> libcamera_base_public_headers, > >>>>> libcamera_base_private_headers, > >>>>> + libcamera_base_details_headers, > >>>>> ] > >>>>> > >>>>> install_headers(libcamera_base_public_headers, > >>>>> subdir : libcamera_base_include_dir) > >>>>> +install_headers(libcamera_base_details_headers, > >>>>> + subdir : libcamera_base_include_dir / 'details') > >>> > >>> ... this seems to indicate the contrary ? Barnabás, could you please > >>> explain what your plan is ? > >>> > >>> If we want to use this in the public API, I think we should shorten the > >>> namespace for public classes, possibly to libcamera::cxx20. If it's > >>> purely internal, we can't move the span class here, and the headers > >>> should probably not be installed. > >> > >> The plan is that these are not part of the public API, merely implementation > >> details that need to be available in public header files, so that they can > >> be used from other public libcamera header files. Applications are not supposed > >> to use them. > > > > Then unless I'm mistaken you don't need to install those headers. I also > > wouldn't put this in a "details" namespace, libcamera::cxx20::* should > > be fine. > > They need to be installed because the plan is that public libcamera headers > files will include them. Ah, then they *are* part of the public API :-) I think you can jsut add those headers to libcamera_base_public_headers.
2025. 07. 27. 20:56 keltezéssel, Laurent Pinchart írta: > On Sun, Jul 27, 2025 at 08:44:31PM +0200, Barnabás Pőcze wrote: >> 2025. 07. 27. 20:37 keltezéssel, Laurent Pinchart írta: >>> On Sun, Jul 27, 2025 at 07:15:56PM +0200, Barnabás Pőcze wrote: >>>> 2025. 07. 27. 15:30 keltezéssel, Laurent Pinchart írta: >>>>> On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: >>>>>> On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: >>>>>>> Add `cxx20.h` that will contain (possibly limited) C++17 implementations >>>>>>> of some C++20 standard library features. >>>>>> >>>>>> Our span.h implementation qualifies in this category as well. >>>>>> >>>>>> As I see it >>>>>> 1) We move span.h in /details/ >>>>>> 2) We move cxx20.h in include/libcamera/base >>>>>> >>>>>> What's your take ? >>>>> >>>>> The issue with our Span class is that it's part of the public API, while >>>>> I understand libcamera::details::cxx20 to be meant for internal usage. >>>>> Although... >>>>> >>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>>> --- >>>>>>> include/libcamera/base/details/cxx20.h | 12 ++++++++++++ >>>>>>> include/libcamera/base/meson.build | 7 +++++++ >>>>>>> 2 files changed, 19 insertions(+) >>>>>>> create mode 100644 include/libcamera/base/details/cxx20.h >>>>>>> >>>>>>> diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h >>>>>>> new file mode 100644 >>>>>>> index 000000000..0d6173d1b >>>>>>> --- /dev/null >>>>>>> +++ b/include/libcamera/base/details/cxx20.h >>>>>>> @@ -0,0 +1,12 @@ >>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>>>>> +/* >>>>>>> + * Copyright (C) 2025, Ideas on Board Oy >>>>>>> + * >>>>>>> + * C++20 polyfills >>>>>>> + */ >>>>>>> + >>>>>>> +#pragma once >>>>>>> + >>>>>>> +namespace libcamera::details::cxx20 { >>>>>>> + >>>>>>> +} /* namespace libcamera::details::cxx20 */ >>>>>>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build >>>>>>> index f28ae4d42..9836daff9 100644 >>>>>>> --- a/include/libcamera/base/meson.build >>>>>>> +++ b/include/libcamera/base/meson.build >>>>>>> @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ >>>>>>> 'utils.h', >>>>>>> ]) >>>>>>> >>>>>>> +libcamera_base_details_headers = files([ >>>>>>> + 'details/cxx20.h', >>>>>>> +]) >>>>>>> + >>>>>>> libcamera_base_headers = [ >>>>>>> libcamera_base_public_headers, >>>>>>> libcamera_base_private_headers, >>>>>>> + libcamera_base_details_headers, >>>>>>> ] >>>>>>> >>>>>>> install_headers(libcamera_base_public_headers, >>>>>>> subdir : libcamera_base_include_dir) >>>>>>> +install_headers(libcamera_base_details_headers, >>>>>>> + subdir : libcamera_base_include_dir / 'details') >>>>> >>>>> ... this seems to indicate the contrary ? Barnabás, could you please >>>>> explain what your plan is ? >>>>> >>>>> If we want to use this in the public API, I think we should shorten the >>>>> namespace for public classes, possibly to libcamera::cxx20. If it's >>>>> purely internal, we can't move the span class here, and the headers >>>>> should probably not be installed. >>>> >>>> The plan is that these are not part of the public API, merely implementation >>>> details that need to be available in public header files, so that they can >>>> be used from other public libcamera header files. Applications are not supposed >>>> to use them. >>> >>> Then unless I'm mistaken you don't need to install those headers. I also >>> wouldn't put this in a "details" namespace, libcamera::cxx20::* should >>> be fine. >> >> They need to be installed because the plan is that public libcamera headers >> files will include them. > > Ah, then they *are* part of the public API :-) > > I think you can jsut add those headers to libcamera_base_public_headers. > I would argue that they are not part of it. They are installed like this because of the limitations of the language, not because these functions/types/etc. are intended to be used by applications, in fact, they should not even be aware of their existence. There are no stability guarantees, etc. of any kind, they may be changed, removed, moved arbitrarily between even point releases.
Hi Barnabás On Sun, Jul 27, 2025 at 09:02:10PM +0200, Barnabás Pőcze wrote: > 2025. 07. 27. 20:56 keltezéssel, Laurent Pinchart írta: > > On Sun, Jul 27, 2025 at 08:44:31PM +0200, Barnabás Pőcze wrote: > > > 2025. 07. 27. 20:37 keltezéssel, Laurent Pinchart írta: > > > > On Sun, Jul 27, 2025 at 07:15:56PM +0200, Barnabás Pőcze wrote: > > > > > 2025. 07. 27. 15:30 keltezéssel, Laurent Pinchart írta: > > > > > > On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: > > > > > > > On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: > > > > > > > > Add `cxx20.h` that will contain (possibly limited) C++17 implementations > > > > > > > > of some C++20 standard library features. > > > > > > > > > > > > > > Our span.h implementation qualifies in this category as well. > > > > > > > > > > > > > > As I see it > > > > > > > 1) We move span.h in /details/ > > > > > > > 2) We move cxx20.h in include/libcamera/base > > > > > > > > > > > > > > What's your take ? > > > > > > > > > > > > The issue with our Span class is that it's part of the public API, while > > > > > > I understand libcamera::details::cxx20 to be meant for internal usage. > > > > > > Although... > > > > > > > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > > > --- > > > > > > > > include/libcamera/base/details/cxx20.h | 12 ++++++++++++ > > > > > > > > include/libcamera/base/meson.build | 7 +++++++ > > > > > > > > 2 files changed, 19 insertions(+) > > > > > > > > create mode 100644 include/libcamera/base/details/cxx20.h > > > > > > > > > > > > > > > > diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h > > > > > > > > new file mode 100644 > > > > > > > > index 000000000..0d6173d1b > > > > > > > > --- /dev/null > > > > > > > > +++ b/include/libcamera/base/details/cxx20.h > > > > > > > > @@ -0,0 +1,12 @@ > > > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > > > > +/* > > > > > > > > + * Copyright (C) 2025, Ideas on Board Oy > > > > > > > > + * > > > > > > > > + * C++20 polyfills > > > > > > > > + */ > > > > > > > > + > > > > > > > > +#pragma once > > > > > > > > + > > > > > > > > +namespace libcamera::details::cxx20 { > > > > > > > > + > > > > > > > > +} /* namespace libcamera::details::cxx20 */ > > > > > > > > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build > > > > > > > > index f28ae4d42..9836daff9 100644 > > > > > > > > --- a/include/libcamera/base/meson.build > > > > > > > > +++ b/include/libcamera/base/meson.build > > > > > > > > @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ > > > > > > > > 'utils.h', > > > > > > > > ]) > > > > > > > > > > > > > > > > +libcamera_base_details_headers = files([ > > > > > > > > + 'details/cxx20.h', > > > > > > > > +]) > > > > > > > > + > > > > > > > > libcamera_base_headers = [ > > > > > > > > libcamera_base_public_headers, > > > > > > > > libcamera_base_private_headers, > > > > > > > > + libcamera_base_details_headers, > > > > > > > > ] > > > > > > > > > > > > > > > > install_headers(libcamera_base_public_headers, > > > > > > > > subdir : libcamera_base_include_dir) > > > > > > > > +install_headers(libcamera_base_details_headers, > > > > > > > > + subdir : libcamera_base_include_dir / 'details') > > > > > > > > > > > > ... this seems to indicate the contrary ? Barnabás, could you please > > > > > > explain what your plan is ? > > > > > > > > > > > > If we want to use this in the public API, I think we should shorten the > > > > > > namespace for public classes, possibly to libcamera::cxx20. If it's > > > > > > purely internal, we can't move the span class here, and the headers > > > > > > should probably not be installed. > > > > > > > > > > The plan is that these are not part of the public API, merely implementation > > > > > details that need to be available in public header files, so that they can > > > > > be used from other public libcamera header files. Applications are not supposed > > > > > to use them. > > > > > > > > Then unless I'm mistaken you don't need to install those headers. I also > > > > wouldn't put this in a "details" namespace, libcamera::cxx20::* should > > > > be fine. > > > > > > They need to be installed because the plan is that public libcamera headers > > > files will include them. > > > > Ah, then they *are* part of the public API :-) > > > > I think you can jsut add those headers to libcamera_base_public_headers. > > > > I would argue that they are not part of it. They are installed like this because > of the limitations of the language, not because these functions/types/etc. are > intended to be used by applications, in fact, they should not even be aware of Isn't the point that applications, once the header has been made available to them, can decide to use it even if we don't suggest them to do so ? > their existence. There are no stability guarantees, etc. of any kind, they may > be changed, removed, moved arbitrarily between even point releases.
2025. 07. 28. 8:37 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Sun, Jul 27, 2025 at 09:02:10PM +0200, Barnabás Pőcze wrote: >> 2025. 07. 27. 20:56 keltezéssel, Laurent Pinchart írta: >>> On Sun, Jul 27, 2025 at 08:44:31PM +0200, Barnabás Pőcze wrote: >>>> 2025. 07. 27. 20:37 keltezéssel, Laurent Pinchart írta: >>>>> On Sun, Jul 27, 2025 at 07:15:56PM +0200, Barnabás Pőcze wrote: >>>>>> 2025. 07. 27. 15:30 keltezéssel, Laurent Pinchart írta: >>>>>>> On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: >>>>>>>> On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: >>>>>>>>> Add `cxx20.h` that will contain (possibly limited) C++17 implementations >>>>>>>>> of some C++20 standard library features. >>>>>>>> >>>>>>>> Our span.h implementation qualifies in this category as well. >>>>>>>> >>>>>>>> As I see it >>>>>>>> 1) We move span.h in /details/ >>>>>>>> 2) We move cxx20.h in include/libcamera/base >>>>>>>> >>>>>>>> What's your take ? >>>>>>> >>>>>>> The issue with our Span class is that it's part of the public API, while >>>>>>> I understand libcamera::details::cxx20 to be meant for internal usage. >>>>>>> Although... >>>>>>> >>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>>>>>> --- >>>>>>>>> include/libcamera/base/details/cxx20.h | 12 ++++++++++++ >>>>>>>>> include/libcamera/base/meson.build | 7 +++++++ >>>>>>>>> 2 files changed, 19 insertions(+) >>>>>>>>> create mode 100644 include/libcamera/base/details/cxx20.h >>>>>>>>> >>>>>>>>> diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h >>>>>>>>> new file mode 100644 >>>>>>>>> index 000000000..0d6173d1b >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/include/libcamera/base/details/cxx20.h >>>>>>>>> @@ -0,0 +1,12 @@ >>>>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>>>>>>> +/* >>>>>>>>> + * Copyright (C) 2025, Ideas on Board Oy >>>>>>>>> + * >>>>>>>>> + * C++20 polyfills >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +#pragma once >>>>>>>>> + >>>>>>>>> +namespace libcamera::details::cxx20 { >>>>>>>>> + >>>>>>>>> +} /* namespace libcamera::details::cxx20 */ >>>>>>>>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build >>>>>>>>> index f28ae4d42..9836daff9 100644 >>>>>>>>> --- a/include/libcamera/base/meson.build >>>>>>>>> +++ b/include/libcamera/base/meson.build >>>>>>>>> @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ >>>>>>>>> 'utils.h', >>>>>>>>> ]) >>>>>>>>> >>>>>>>>> +libcamera_base_details_headers = files([ >>>>>>>>> + 'details/cxx20.h', >>>>>>>>> +]) >>>>>>>>> + >>>>>>>>> libcamera_base_headers = [ >>>>>>>>> libcamera_base_public_headers, >>>>>>>>> libcamera_base_private_headers, >>>>>>>>> + libcamera_base_details_headers, >>>>>>>>> ] >>>>>>>>> >>>>>>>>> install_headers(libcamera_base_public_headers, >>>>>>>>> subdir : libcamera_base_include_dir) >>>>>>>>> +install_headers(libcamera_base_details_headers, >>>>>>>>> + subdir : libcamera_base_include_dir / 'details') >>>>>>> >>>>>>> ... this seems to indicate the contrary ? Barnabás, could you please >>>>>>> explain what your plan is ? >>>>>>> >>>>>>> If we want to use this in the public API, I think we should shorten the >>>>>>> namespace for public classes, possibly to libcamera::cxx20. If it's >>>>>>> purely internal, we can't move the span class here, and the headers >>>>>>> should probably not be installed. >>>>>> >>>>>> The plan is that these are not part of the public API, merely implementation >>>>>> details that need to be available in public header files, so that they can >>>>>> be used from other public libcamera header files. Applications are not supposed >>>>>> to use them. >>>>> >>>>> Then unless I'm mistaken you don't need to install those headers. I also >>>>> wouldn't put this in a "details" namespace, libcamera::cxx20::* should >>>>> be fine. >>>> >>>> They need to be installed because the plan is that public libcamera headers >>>> files will include them. >>> >>> Ah, then they *are* part of the public API :-) >>> >>> I think you can jsut add those headers to libcamera_base_public_headers. >>> >> >> I would argue that they are not part of it. They are installed like this because >> of the limitations of the language, not because these functions/types/etc. are >> intended to be used by applications, in fact, they should not even be aware of > > Isn't the point that applications, once the header has been made available > to them, can decide to use it even if we don't suggest them to do so ? That's true, but it's a limitation of the language. They can certainly use it, but my point is that libcamera should provide no guarantees regarding these parts. Yes, you can use it, but that is not supported. I'd argue that the use of "details" namespace is quite a common pattern, so this shouldn't be a surprise to anyone familiar. > >> their existence. There are no stability guarantees, etc. of any kind, they may >> be changed, removed, moved arbitrarily between even point releases.
Quoting Barnabás Pőcze (2025-08-15 18:04:21) > 2025. 07. 28. 8:37 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Sun, Jul 27, 2025 at 09:02:10PM +0200, Barnabás Pőcze wrote: > >> 2025. 07. 27. 20:56 keltezéssel, Laurent Pinchart írta: > >>> On Sun, Jul 27, 2025 at 08:44:31PM +0200, Barnabás Pőcze wrote: > >>>> 2025. 07. 27. 20:37 keltezéssel, Laurent Pinchart írta: > >>>>> On Sun, Jul 27, 2025 at 07:15:56PM +0200, Barnabás Pőcze wrote: > >>>>>> 2025. 07. 27. 15:30 keltezéssel, Laurent Pinchart írta: > >>>>>>> On Fri, Jul 25, 2025 at 02:58:06PM +0200, Jacopo Mondi wrote: > >>>>>>>> On Mon, Jul 21, 2025 at 12:46:03PM +0200, Barnabás Pőcze wrote: > >>>>>>>>> Add `cxx20.h` that will contain (possibly limited) C++17 implementations > >>>>>>>>> of some C++20 standard library features. > >>>>>>>> > >>>>>>>> Our span.h implementation qualifies in this category as well. > >>>>>>>> > >>>>>>>> As I see it > >>>>>>>> 1) We move span.h in /details/ > >>>>>>>> 2) We move cxx20.h in include/libcamera/base > >>>>>>>> > >>>>>>>> What's your take ? > >>>>>>> > >>>>>>> The issue with our Span class is that it's part of the public API, while > >>>>>>> I understand libcamera::details::cxx20 to be meant for internal usage. > >>>>>>> Although... > >>>>>>> > >>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>>>>>>> --- > >>>>>>>>> include/libcamera/base/details/cxx20.h | 12 ++++++++++++ > >>>>>>>>> include/libcamera/base/meson.build | 7 +++++++ > >>>>>>>>> 2 files changed, 19 insertions(+) > >>>>>>>>> create mode 100644 include/libcamera/base/details/cxx20.h > >>>>>>>>> > >>>>>>>>> diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h > >>>>>>>>> new file mode 100644 > >>>>>>>>> index 000000000..0d6173d1b > >>>>>>>>> --- /dev/null > >>>>>>>>> +++ b/include/libcamera/base/details/cxx20.h > >>>>>>>>> @@ -0,0 +1,12 @@ > >>>>>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >>>>>>>>> +/* > >>>>>>>>> + * Copyright (C) 2025, Ideas on Board Oy > >>>>>>>>> + * > >>>>>>>>> + * C++20 polyfills > >>>>>>>>> + */ > >>>>>>>>> + > >>>>>>>>> +#pragma once > >>>>>>>>> + > >>>>>>>>> +namespace libcamera::details::cxx20 { > >>>>>>>>> + > >>>>>>>>> +} /* namespace libcamera::details::cxx20 */ > >>>>>>>>> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build > >>>>>>>>> index f28ae4d42..9836daff9 100644 > >>>>>>>>> --- a/include/libcamera/base/meson.build > >>>>>>>>> +++ b/include/libcamera/base/meson.build > >>>>>>>>> @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ > >>>>>>>>> 'utils.h', > >>>>>>>>> ]) > >>>>>>>>> > >>>>>>>>> +libcamera_base_details_headers = files([ > >>>>>>>>> + 'details/cxx20.h', > >>>>>>>>> +]) > >>>>>>>>> + > >>>>>>>>> libcamera_base_headers = [ > >>>>>>>>> libcamera_base_public_headers, > >>>>>>>>> libcamera_base_private_headers, > >>>>>>>>> + libcamera_base_details_headers, > >>>>>>>>> ] > >>>>>>>>> > >>>>>>>>> install_headers(libcamera_base_public_headers, > >>>>>>>>> subdir : libcamera_base_include_dir) > >>>>>>>>> +install_headers(libcamera_base_details_headers, > >>>>>>>>> + subdir : libcamera_base_include_dir / 'details') > >>>>>>> > >>>>>>> ... this seems to indicate the contrary ? Barnabás, could you please > >>>>>>> explain what your plan is ? > >>>>>>> > >>>>>>> If we want to use this in the public API, I think we should shorten the > >>>>>>> namespace for public classes, possibly to libcamera::cxx20. If it's > >>>>>>> purely internal, we can't move the span class here, and the headers > >>>>>>> should probably not be installed. > >>>>>> > >>>>>> The plan is that these are not part of the public API, merely implementation > >>>>>> details that need to be available in public header files, so that they can > >>>>>> be used from other public libcamera header files. Applications are not supposed > >>>>>> to use them. > >>>>> > >>>>> Then unless I'm mistaken you don't need to install those headers. I also > >>>>> wouldn't put this in a "details" namespace, libcamera::cxx20::* should > >>>>> be fine. > >>>> > >>>> They need to be installed because the plan is that public libcamera headers > >>>> files will include them. > >>> > >>> Ah, then they *are* part of the public API :-) > >>> > >>> I think you can jsut add those headers to libcamera_base_public_headers. > >>> > >> > >> I would argue that they are not part of it. They are installed like this because > >> of the limitations of the language, not because these functions/types/etc. are > >> intended to be used by applications, in fact, they should not even be aware of > > > > Isn't the point that applications, once the header has been made available > > to them, can decide to use it even if we don't suggest them to do so ? > > That's true, but it's a limitation of the language. They can certainly use it, > but my point is that libcamera should provide no guarantees regarding these parts. > Yes, you can use it, but that is not supported. I'd argue that the use of "details" > namespace is quite a common pattern, so this shouldn't be a surprise to anyone familiar. Ok, so the public libcamera headers require them, therefore cxx20.h needs to be in the public headers. Since they are in the public headers, applications are able use them regardless of if we tell them they cannot use it. This is the premise. So do we consider cxx20.h to be part of the public API that needs to be checked for changes every time we make a release? And if it changes then we need to bump the version number etc etc. > > > > >> their existence. There are no stability guarantees, etc. of any kind, they may > >> be changed, removed, moved arbitrarily between even point releases. > Or are we permitted to change it anytime we want and just say "deal with it" to applications that decided to use it against our policy? I personally don't think that the latter is reasonable. We exposed it as a public API whether it was intended to be used by applications or not. Therefore it's probably more reasonable (and responsible) to go the full mile and just expose it as public API and include it in public API versioning/policies. I understand that it's due to a limitation of the language, but with this direction in design, I think it's the more responsible choice to make it fully part of the public API, just like how spans are. Also considering that the API surface isn't that big and I don't expect it to change very frequently (maybe grow but not change) I don't think it'll be that big of an issue to support properly. My two cents. Paul
diff --git a/include/libcamera/base/details/cxx20.h b/include/libcamera/base/details/cxx20.h new file mode 100644 index 000000000..0d6173d1b --- /dev/null +++ b/include/libcamera/base/details/cxx20.h @@ -0,0 +1,12 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2025, Ideas on Board Oy + * + * C++20 polyfills + */ + +#pragma once + +namespace libcamera::details::cxx20 { + +} /* namespace libcamera::details::cxx20 */ diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build index f28ae4d42..9836daff9 100644 --- a/include/libcamera/base/meson.build +++ b/include/libcamera/base/meson.build @@ -31,10 +31,17 @@ libcamera_base_private_headers = files([ 'utils.h', ]) +libcamera_base_details_headers = files([ + 'details/cxx20.h', +]) + libcamera_base_headers = [ libcamera_base_public_headers, libcamera_base_private_headers, + libcamera_base_details_headers, ] install_headers(libcamera_base_public_headers, subdir : libcamera_base_include_dir) +install_headers(libcamera_base_details_headers, + subdir : libcamera_base_include_dir / 'details')
Add `cxx20.h` that will contain (possibly limited) C++17 implementations of some C++20 standard library features. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/base/details/cxx20.h | 12 ++++++++++++ include/libcamera/base/meson.build | 7 +++++++ 2 files changed, 19 insertions(+) create mode 100644 include/libcamera/base/details/cxx20.h