[libcamera-devel] Usage of C++17 in the public API (was "[PATCH] cam: convert to libfmt")

[Christian Rauch]

Hi Laurent,

Am 30.05.22 um 11:37 schrieb Laurent Pinchart:
> Hi Christian,
>
> On Mon, May 30, 2022 at 11:11:23AM +0100, Christian Rauch wrote:
>> Am 30.05.22 um 10:03 schrieb Laurent Pinchart:
>>> On Mon, May 30, 2022 at 09:47:25AM +0100, Eric Curtin via libcamera-devel wrote:
>>>> On Sat, 28 May 2022 at 00:19, Christian Rauch wrote:
>>>>> Am 22.05.22 um 11:52 schrieb Laurent Pinchart via libcamera-devel:
>>>>>> On Sun, May 22, 2022 at 11:08:27AM +0100, Kieran Bingham wrote:
>>>>>>> Quoting Laurent Pinchart (2022-05-10 14:04:57)
>>>>>>>> On Tue, May 10, 2022 at 01:59:37PM +0100, Eric Curtin via libcamera-devel wrote:
>>>>>>>>> On Tue, 10 May 2022 at 11:10, Kieran Bingham via libcamera-devel wrote:
>>>>>>>>>> Quoting Naushir Patuck (2022-05-10 10:49:14)
>>>>>>>>>>> On Tue, 10 May 2022 at 08:16, Tomi Valkeinen via libcamera-devel wrote:
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>>>>>> For what it's worth, I absolutely loathe formatting in std iostream, and
>>>>>>>>>>> libfmt is a wonderful alternative.
>>>>>>>>>>> It also is close enough to the C++20 std::format implementation that
>>>>>>>>>>> eventual porting would be low effort.  So I am all for this change :)
>>>>>>>>
>>>>>>>> It's going to take a while before we can switch to C++20, but I'm
>>>>>>>> looking forward to it (and before that to some of the C++17 features
>>>>>>>> too, it would be nice to use std::optional in the public API).
>>>>>>>
>>>>>>> So, I've just seen as Christian said - we already do!
>>>>>>>
>>>>>>> It's used by color space ..
>>>>>>
>>>>>> Oops...
>>>>>>
>>>>>>> So we're faced with either reverting, or rolling back on the color-space
>>>>>>> change, or pushing forwards with C++17 on the public-api.
>>>>>>
>>>>>> Switching to C++17 worries me as it's fairly recent and may not be
>>>>>> easily available for all users. std::optional could easily rbe replaced
>>>>>> with a custom implementation in utils::optional if needed. I'm tempted
>>>>>> to leave std::optional in for now, and accept new users of std::optional
>>>>>> but no other C++17 APIs. Switching to utils::optional for ColorSpace
>>>>>> would cover new users of std::optional so we're not creating any new
>>>>>> liability.
>>>>>
>>>>> I think that C++17 is quite old by now (nearly 5 years) and all major
>>>>> compilers support it for multiple versions and years now. Do you know of
>>>>> an example, where requiring C++17 would hinder the adaptation of
>>>>> libcamera? Do any of the target system not support this standard?
>>>
>>> The main blocker used to tbe Chromium, which used C++14. It seems to
>>> have moved to C++17 now, and we have also dropped the plan for direct
>>> integration of libcamera support in Chromium (and Firefox) as they
>>> should go through PipeWire and the XDG camera portal.
>>>
>>>>> I think the use of std::optional to express invalid values is quite
>>>>> significant compared to the previous solution of using default
>>>>> constructed values. Reimplementing this in libcamera would be a
>>>>> solution, but it just adds development and maintenance costs for no
>>>>> obvious reason (to me).
>>>
>>> We can use std::optional (as it's there already). *If* C++17 support
>>> ends up being an issue in the future, requiring us to move back to
>>> C++14, then it will be easy to make a custom implementation, but only if
>>> necessary (we've done that with std::span already, which is only
>>> available in C++20).
>>>
>>>>>>> Maybe this deserves it's own thread...
>>>>>>
>>>>>> Done :-)
>>>>>>
>>>>>>>>> I am all in for this change also, personally I would have changed to
>>>>>>>>> printf for now
>>>>>>>>> to have one less dependency (also an easy port to C++20 std::format). The
>>>>>>>>> dependency list is getting large, I noticed a build of mine failing
>>>>>>>>> recently because
>>>>>>>>> I didn't have libyaml.
>>>>>>>>
>>>>>>>> I've posted a patch that falls back to a subproject in that case. Would
>>>>>>>> you be able to give it a try ?
>>>>>>>>
>>>>>>>>> But std::format and libfmt are quite fast and anything is better than
>>>>>>>>> streams so +1.
>>>>>>>>>
>>>>>>>>>> I've never used (yet) {fmt}, but I've only heard good things about
>>>>>>>>>> performance, and of course it's headed into the standard, so I also
>>>>>>>>>> think there is some good merit to be found in this development.
>>>>
>>>> Only speaking for the red hat distros, every distro that has a package
>>>> built for libcamera or could have in future has at least C++17,
>>>> including newly released LTS release RHEL9.
>>>
>>> That's good to know. The other question is whether all C++ applications
>>> that may want to use libcamera can be compiled with C++17 or newer.
>>
>> Isn't C++17 backwards compatible? My understanding is that every earlier
>> C++ standard (e.g. C++11) should be able to compile with a C++17 (or
>> later) compiler.
>
> Not quite I'm afraid.
>
> --------
> #include <map>	/* This includes <optional> indirectly in libc++ */
>
> using namespace std;
>
> template<class T>
> class optional
> {
> public:
>         optional()
>         {
>         }
> };
>
> int main()
> {
>         optional<int> opt;
>
>         return 0;
> }

Right, this is clashing because of ambiguous symbols from different
namespaces. You have a point there. I only thought in terms of API
compatibility there.

Anyway, libcamera already requires C++17 and uses std::optional. I would
like to push for a decision if libcamera:
A) continues to use C++17 and std::optional
B) reverts those changes and implements a custom "optional"

I am in favour of keeping the status quo of using std::optional. The
reason is that I think that not many projects (if any) will be affected
by these kinds of ambiguity issues above. Resorting to a custom
solution, just to switch to std::optional in the public API later
(when?), will cause more breakage issues. A custom temporary solution
will also require someone to implement and maintain this, knowing that
it will be replaced eventually anyway.

I simply think that the benefits outweigh the potential issues. Using
std::optional, for example as control value return type, improves the
descriptiveness of the API. While on the other side, there seem to be no
projects struggling with the current C++17/std::optional situation.

Sorry for pushing for a solution, but I really would like to continue
with my original patch set :-)

Best,
Christian

> --------
>
> $ clang++ -W -Wall -stdlib=libc++ -std=c++14 -o version version.cpp
> $ clang++ -W -Wall -stdlib=libc++ -std=c++17 -o version version.cpp
> version.cpp:16:2: error: reference to 'optional' is ambiguous
>         optional<int> opt;
>         ^
> version.cpp:6:7: note: candidate found by name lookup is 'optional'
> class optional
>       ^
> /usr/include/c++/v1/optional:590:7: note: candidate found by name lookup is 'std::optional'
> class optional
>       ^
> 1 error generated.
>
>>>> Given libcamera is still 0.0 it could be a nice opportunity to take.
>