Re: [libgpiod v2][PATCH v6 2/5] bindings: cxx: add v2 headers

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Apr 26, 2022 at 2:50 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> This adds the headers for the v2 C++ bindings.
>
> Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>

I'm pasting below the review I got from my work colleague Bernhard
Rosenkränzer (Cc'ed):

Bart

---

Looks good to me, one minor nit: I presume bindings/cxx/gpiod.hpp Is
supposed to be #include-d by users of the library. The definition of
GPIOD_CXX_API in the file causes the symbol to be re-exported by
anything including the header. Usually the way to achieve what you
want there is

AM_CPPFLAGS += -DBUILDING_GPIOD=1
in Makefile.am, and the definition of GPIOD_CXX_API changed to

#ifdef BUILDING_GPIOD
#define GPIOD_CXX_API __attribute__((visibility("default")))
#else
#define GPIOD_CXX_API __attribute__((visibility("hidden")))
#endif

Also, in Makefile.am you probably want to change
libgpiodcxx_la_CPPFLAGS to libgpiodcxx_la_CXXFLAGS -- CPPFLAGS is for
C/C++ PreProcessor flags, CXXFLAGS is for the C++ compiler

Doesn't make much of a difference currently because at least with
clang and gcc the preprocessor isn't called separately, but you never
know what kind of insanity a future automake version will bring

You might also want to change the various methods taking (void) [none]
as parameters to just taking () -- (void) is a C-ism, not needed for
C++ (and you never know if at some point the standard will actually
disallow it)

Also, I'm not sure if it's intentional that some classes (class chip
etc.) aren't marked GPIOD_CXX_API

line-config.hpp uses using override = ::std::pair<line::offset,
property>; -- override is a reserved word (to be used by something
inheriting a class to indicate the method being defined overrides a
base class method -- optional, what it actually does is cause a
compile time error if parameters mismatch or there's some other reason
why it doesn't overload something), so using it in this context might
break with some compilers

Minor nit: some parameters could add more const-ness (e.g. void
clear_drive_override(*const* line::offset offset) noexcept;)

In theory, this could help a compiler optimize something out, but in
practice I don't know any compiler that would actually generate better
code from that