[libcamera-devel] [PATCH v3 1/8] libcamera: utils: Define BIT() macro

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Apr 08, 2019 at 09:46:37AM +0200, Jacopo Mondi wrote:
> On Fri, Apr 05, 2019 at 02:31:13PM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 03, 2019 at 05:07:28PM +0200, Jacopo Mondi wrote:
> >> Define BIT(b_) macro which expands to a left bit shift.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >> ---
> >>  src/libcamera/include/utils.h | 1 +
> >>  src/libcamera/utils.cpp       | 5 +++++
> >>  2 files changed, 6 insertions(+)
> >>
> >> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> >> index 79038a96feab..1a6cf7f7b9dc 100644
> >> --- a/src/libcamera/include/utils.h
> >> +++ b/src/libcamera/include/utils.h
> >> @@ -10,6 +10,7 @@
> >>  #include <memory>
> >>
> >>  #define ARRAY_SIZE(a)	(sizeof(a) / sizeof(a[0]))
> >> +#define BIT(b_)		(1 << (b_))
> >
> > I'm curious, why the _ ? I'm also curious, why do you think we need this
> 
> To avoid (unlikely) name clashes with other 'b' variables?

I don't think that's needed, the compiler will not mistake the macro
argument name with local variables named 'b'.

> > macro ? :-) Usage of BIT() in the Linux kernel is partly to fix problems
> > with (1 << 31) on 32-bit platforms, with BIT() defined as (1UL << (nr)).
> 
> I just wanted something to avoid typing (1 << x) all the times, and
> we're all used to the BIT() macro, so...
> 
> I could drop this if it feels not necessary...

I'm OK keeping it if you think that's useful, but I think you should
then at least replace 1 with 1UL for the reason above.

> >>  namespace libcamera {
> >>
> >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> >> index cd0fd7614cc7..1a5a2a03b1ca 100644
> >> --- a/src/libcamera/utils.cpp
> >> +++ b/src/libcamera/utils.cpp
> >> @@ -24,6 +24,11 @@ namespace utils {
> >>   * \brief Determine the number of elements in the static array.
> >>   */
> >>
> >> +/**
> >> + * \def BIT(b)
> >> + * \brief Bitwise left shift by \a b bits
> >
> > Bitwise left shift of what ? :-)
> >
> >> + */
> >> +
> >>  /**
> >>   * \brief Strip the directory prefix from the path
> >>   * \param[in] path The path to process

-- 
Regards,

Laurent Pinchart