libcamera: base: Don't redefine __nodiscard

22452 diff mbox series

Some libc implementations (like bionic) already define the __nodiscard
macro [1].

Building with bionic results in compiler error since libcamera redefines
the __nodiscard macro.

Don't redefine __nodiscard if it's already defined.

[1] https://android-review.googlesource.com/c/platform/bionic/+/3254860

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
Hi, it's been a while. I've found a (trivial) build issue when
building against recent bionic versions.

I hope this is the right fix. Thanks for your review!
---
 include/libcamera/base/compiler.h | 2 ++
 1 file changed, 2 insertions(+)


---
base-commit: 35ed4b91291d9f3d08e4b51acfb51163e65df8f8
change-id: 20250103-nodiscard-redef-9158e8fdc3f5

Best regards,

Hi Mattijs,

Thank you for the patch.

On Fri, Jan 03, 2025 at 03:02:50PM +0100, Mattijs Korpershoek wrote:
> Some libc implementations (like bionic) already define the __nodiscard
> macro [1].
> 
> Building with bionic results in compiler error since libcamera redefines
> the __nodiscard macro.
> 
> Don't redefine __nodiscard if it's already defined.

bionic defines it as __attribute__((__warn_unused_result__)) which
(hopefully) does the exact same thing as [[nodiscard]]. Someone else
could define it differently though. I suppose the risk is low ?

> [1] https://android-review.googlesource.com/c/platform/bionic/+/3254860
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> Hi, it's been a while. I've found a (trivial) build issue when
> building against recent bionic versions.
> 
> I hope this is the right fix. Thanks for your review!

It's been nearly four years since we introduced __nodiscard. I wonder if
we could require applications to use C++17 or newer now, and use
[[nodiscard]] directly.

> ---
>  include/libcamera/base/compiler.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/libcamera/base/compiler.h b/include/libcamera/base/compiler.h
> index fda8fdfdc543f86c5554e38ef790c00d72d60389..946e20d835b216446e4099b6ab6865638d9aaf26 100644
> --- a/include/libcamera/base/compiler.h
> +++ b/include/libcamera/base/compiler.h
> @@ -7,8 +7,10 @@
>  
>  #pragma once
>  
> +#ifndef __nodiscard
>  #if __cplusplus >= 201703L
>  #define __nodiscard		[[nodiscard]]
>  #else
>  #define __nodiscard
>  #endif
> +#endif
> 
> ---
> base-commit: 35ed4b91291d9f3d08e4b51acfb51163e65df8f8
> change-id: 20250103-nodiscard-redef-9158e8fdc3f5

Hi Laurent,

Thank you for the review.

On ven., janv. 03, 2025 at 16:53, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Mattijs,
>
> Thank you for the patch.
>
> On Fri, Jan 03, 2025 at 03:02:50PM +0100, Mattijs Korpershoek wrote:
>> Some libc implementations (like bionic) already define the __nodiscard
>> macro [1].
>> 
>> Building with bionic results in compiler error since libcamera redefines
>> the __nodiscard macro.
>> 
>> Don't redefine __nodiscard if it's already defined.
>
> bionic defines it as __attribute__((__warn_unused_result__)) which
> (hopefully) does the exact same thing as [[nodiscard]]. Someone else
> could define it differently though. I suppose the risk is low ?

My gut feeling is that the risk is pretty low.

>
>> [1] https://android-review.googlesource.com/c/platform/bionic/+/3254860
>> 
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> ---
>> Hi, it's been a while. I've found a (trivial) build issue when
>> building against recent bionic versions.
>> 
>> I hope this is the right fix. Thanks for your review!
>
> It's been nearly four years since we introduced __nodiscard. I wonder if
> we could require applications to use C++17 or newer now, and use
> [[nodiscard]] directly.

I've done a quick sed to use [[nodiscard]] directly and seems to build
fine as well on android-15.0.0_r7 (AP4A.241205.013).

Given that meson.build has cpp_std=c++17 and that [[nodiscard]] has been
introduced in C++17, should I send a v2 to get rid of __nodiscard and
compiler.h ?

>
>> ---
>>  include/libcamera/base/compiler.h | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/include/libcamera/base/compiler.h b/include/libcamera/base/compiler.h
>> index fda8fdfdc543f86c5554e38ef790c00d72d60389..946e20d835b216446e4099b6ab6865638d9aaf26 100644
>> --- a/include/libcamera/base/compiler.h
>> +++ b/include/libcamera/base/compiler.h
>> @@ -7,8 +7,10 @@
>>  
>>  #pragma once
>>  
>> +#ifndef __nodiscard
>>  #if __cplusplus >= 201703L
>>  #define __nodiscard		[[nodiscard]]
>>  #else
>>  #define __nodiscard
>>  #endif
>> +#endif
>> 
>> ---
>> base-commit: 35ed4b91291d9f3d08e4b51acfb51163e65df8f8
>> change-id: 20250103-nodiscard-redef-9158e8fdc3f5
>
> -- 
> Regards,
>
> Laurent Pinchart

Hi Mattijs,

On Fri, Jan 03, 2025 at 05:30:50PM +0100, Mattijs Korpershoek wrote:
> On ven., janv. 03, 2025 at 16:53, Laurent Pinchart wrote:
> > On Fri, Jan 03, 2025 at 03:02:50PM +0100, Mattijs Korpershoek wrote:
> >> Some libc implementations (like bionic) already define the __nodiscard
> >> macro [1].
> >> 
> >> Building with bionic results in compiler error since libcamera redefines
> >> the __nodiscard macro.
> >> 
> >> Don't redefine __nodiscard if it's already defined.
> >
> > bionic defines it as __attribute__((__warn_unused_result__)) which
> > (hopefully) does the exact same thing as [[nodiscard]]. Someone else
> > could define it differently though. I suppose the risk is low ?
> 
> My gut feeling is that the risk is pretty low.

Our guts agree.

> >> [1] https://android-review.googlesource.com/c/platform/bionic/+/3254860
> >> 
> >> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> >> ---
> >> Hi, it's been a while. I've found a (trivial) build issue when
> >> building against recent bionic versions.
> >> 
> >> I hope this is the right fix. Thanks for your review!
> >
> > It's been nearly four years since we introduced __nodiscard. I wonder if
> > we could require applications to use C++17 or newer now, and use
> > [[nodiscard]] directly.
> 
> I've done a quick sed to use [[nodiscard]] directly and seems to build
> fine as well on android-15.0.0_r7 (AP4A.241205.013).
> 
> Given that meson.build has cpp_std=c++17 and that [[nodiscard]] has been
> introduced in C++17, should I send a v2 to get rid of __nodiscard and
> compiler.h ?

The question is whether or not anyone still uses libcamera in a project
compiled with C++14. I have tried compiling simple-cam with
cpp_std=c++14 and it failed due to usage of C++17 features in multiple
locations in the libcamera public headers. It should be safe to assume
nobody uses libcamera with C++14 anymore. Dropping compiler.h therefore
seems best to me.

It would be nice if you could send a v2.

> >> ---
> >>  include/libcamera/base/compiler.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/include/libcamera/base/compiler.h b/include/libcamera/base/compiler.h
> >> index fda8fdfdc543f86c5554e38ef790c00d72d60389..946e20d835b216446e4099b6ab6865638d9aaf26 100644
> >> --- a/include/libcamera/base/compiler.h
> >> +++ b/include/libcamera/base/compiler.h
> >> @@ -7,8 +7,10 @@
> >>  
> >>  #pragma once
> >>  
> >> +#ifndef __nodiscard
> >>  #if __cplusplus >= 201703L
> >>  #define __nodiscard		[[nodiscard]]
> >>  #else
> >>  #define __nodiscard
> >>  #endif
> >> +#endif
> >> 
> >> ---
> >> base-commit: 35ed4b91291d9f3d08e4b51acfb51163e65df8f8
> >> change-id: 20250103-nodiscard-redef-9158e8fdc3f5

Hi Laurent,

On lun., janv. 06, 2025 at 10:50, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Mattijs,
>
> On Fri, Jan 03, 2025 at 05:30:50PM +0100, Mattijs Korpershoek wrote:
>> On ven., janv. 03, 2025 at 16:53, Laurent Pinchart wrote:
>> > On Fri, Jan 03, 2025 at 03:02:50PM +0100, Mattijs Korpershoek wrote:
>> >> Some libc implementations (like bionic) already define the __nodiscard
>> >> macro [1].
>> >> 
>> >> Building with bionic results in compiler error since libcamera redefines
>> >> the __nodiscard macro.
>> >> 
>> >> Don't redefine __nodiscard if it's already defined.
>> >
>> > bionic defines it as __attribute__((__warn_unused_result__)) which
>> > (hopefully) does the exact same thing as [[nodiscard]]. Someone else
>> > could define it differently though. I suppose the risk is low ?
>> 
>> My gut feeling is that the risk is pretty low.
>
> Our guts agree.
>
>> >> [1] https://android-review.googlesource.com/c/platform/bionic/+/3254860
>> >> 
>> >> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> >> ---
>> >> Hi, it's been a while. I've found a (trivial) build issue when
>> >> building against recent bionic versions.
>> >> 
>> >> I hope this is the right fix. Thanks for your review!
>> >
>> > It's been nearly four years since we introduced __nodiscard. I wonder if
>> > we could require applications to use C++17 or newer now, and use
>> > [[nodiscard]] directly.
>> 
>> I've done a quick sed to use [[nodiscard]] directly and seems to build
>> fine as well on android-15.0.0_r7 (AP4A.241205.013).
>> 
>> Given that meson.build has cpp_std=c++17 and that [[nodiscard]] has been
>> introduced in C++17, should I send a v2 to get rid of __nodiscard and
>> compiler.h ?
>
> The question is whether or not anyone still uses libcamera in a project
> compiled with C++14. I have tried compiling simple-cam with
> cpp_std=c++14 and it failed due to usage of C++17 features in multiple
> locations in the libcamera public headers. It should be safe to assume
> nobody uses libcamera with C++14 anymore. Dropping compiler.h therefore
> seems best to me.
>
> It would be nice if you could send a v2.

Thanks, done here:

https://lists.libcamera.org/pipermail/libcamera-devel/2025-January/047937.html

>
>> >> ---
>> >>  include/libcamera/base/compiler.h | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >> 
>> >> diff --git a/include/libcamera/base/compiler.h b/include/libcamera/base/compiler.h
>> >> index fda8fdfdc543f86c5554e38ef790c00d72d60389..946e20d835b216446e4099b6ab6865638d9aaf26 100644
>> >> --- a/include/libcamera/base/compiler.h
>> >> +++ b/include/libcamera/base/compiler.h
>> >> @@ -7,8 +7,10 @@
>> >>  
>> >>  #pragma once
>> >>  
>> >> +#ifndef __nodiscard
>> >>  #if __cplusplus >= 201703L
>> >>  #define __nodiscard		[[nodiscard]]
>> >>  #else
>> >>  #define __nodiscard
>> >>  #endif
>> >> +#endif
>> >> 
>> >> ---
>> >> base-commit: 35ed4b91291d9f3d08e4b51acfb51163e65df8f8
>> >> change-id: 20250103-nodiscard-redef-9158e8fdc3f5
>
> -- 
> Regards,
>
> Laurent Pinchart