libgpiod v2: C++ bindings

This series contains the implementation of the C++ bindings for libgpiod v2. In general the C++ library follows the data structure model as defined by the C library with one notable exception: objects that represent immutable snapshots of kernel data (line_info and edge & info events) are copyable (or rather shared behind the scenes using ::std::shared_ptr). The rest of the classes delete their copy constructors and assignment operators and are only move constructible and move assignable.

All classes follow the pimpl idiom - using either shared_ptr or unique_ptr - and all implementations are hidden from the user for easier maintenance and less ABI breakage in the future.

The edge_event class is a bit of a special case. While it looks the same as other copyable objects to the user, the implementation uses a tiny bit of polymorphism (although it never crosses the ABI boundary). This is done to make it possible to use the edge_event_buffer without any memory allocations like what the C API enables. The edge_event objects stored in the buffer only contain a raw pointer to the C object stored in the underlying C edge_event_buffer. The event is copied into a fully managed object once the copy assignment operator is called.

I'm Cc'ing people who showed interest and helped me with C++ bindings before for review.

v1 -> v2:
Kent: I addressed most points from your review. Some are unaddressed due to personal preference (for instance: I still allow creating of empty line-request objects as they may be reused in subsequent requests). I also kept the 'watch' argument in get_line_info() as well as the boolean operators for chip and request - although with (hopefully) better documentation.

The first two patches extend the core C API of libgpiod as discussed.

Bartosz Golaszewski (3):
API: add a function for retrieving the capacity of edge event buffers
API: extend the line request functionality
bindings: cxx: implement C++ bindings for libgpiod v2.0

 Doxyfile.in                                 |   4 +-
 bindings/cxx/Makefile.am                    |  16 +-
 bindings/cxx/chip.cpp                       | 214 +++--
 bindings/cxx/edge-event-buffer.cpp          | 103 +++
 bindings/cxx/edge-event.cpp                 | 123 +++
 bindings/cxx/examples/Makefile.am           |  12 +-
 bindings/cxx/examples/gpiodetectcxx.cpp     |   3 +-
 bindings/cxx/examples/gpiogetcxx.cpp        |  12 +-
 bindings/cxx/examples/gpioinfocxx.cpp       |  63 +-
 bindings/cxx/examples/gpiomoncxx.cpp        |  39 +-
 bindings/cxx/examples/gpiosetcxx.cpp        |  19 +-
 bindings/cxx/gpiod.hpp                      | 938 +-------------------
 bindings/cxx/gpiodcxx/Makefile.am           |  14 +
 bindings/cxx/gpiodcxx/chip.hpp              | 180 ++++
 bindings/cxx/gpiodcxx/edge-event-buffer.hpp | 115 +++
 bindings/cxx/gpiodcxx/edge-event.hpp        | 124 +++
 bindings/cxx/gpiodcxx/info-event.hpp        | 107 +++
 bindings/cxx/gpiodcxx/line-config.hpp       | 244 +++++
 bindings/cxx/gpiodcxx/line-info.hpp         | 205 +++++
 bindings/cxx/gpiodcxx/line-request.hpp      | 207 +++++
 bindings/cxx/gpiodcxx/misc.hpp              |  49 +
 bindings/cxx/gpiodcxx/request-config.hpp    |  97 ++
 bindings/cxx/info-event.cpp                 |  89 ++
 bindings/cxx/internal.hpp                   | 168 +++-
 bindings/cxx/iter.cpp                       |  60 --
 bindings/cxx/line-config.cpp                | 226 +++++
 bindings/cxx/line-info.cpp                  | 150 ++++
 bindings/cxx/line-request.cpp               | 194 ++++
 bindings/cxx/line.cpp                       | 321 -------
 bindings/cxx/line_bulk.cpp                  | 366 --------
 bindings/cxx/misc.cpp                       |  18 +
 bindings/cxx/request-config.cpp             |  80 ++
 configure.ac                                |   1 +
 include/gpiod.h                             |  63 +-
 lib/edge-event.c                            |   6 +
 lib/line-request.c                          |  50 +-
 tools/gpioget.c                             |   3 +-
 37 files changed, 2838 insertions(+), 1845 deletions(-)
 create mode 100644 bindings/cxx/edge-event-buffer.cpp
 create mode 100644 bindings/cxx/edge-event.cpp
 create mode 100644 bindings/cxx/gpiodcxx/Makefile.am
 create mode 100644 bindings/cxx/gpiodcxx/chip.hpp
 create mode 100644 bindings/cxx/gpiodcxx/edge-event-buffer.hpp
 create mode 100644 bindings/cxx/gpiodcxx/edge-event.hpp
 create mode 100644 bindings/cxx/gpiodcxx/info-event.hpp
 create mode 100644 bindings/cxx/gpiodcxx/line-config.hpp
 create mode 100644 bindings/cxx/gpiodcxx/line-info.hpp
 create mode 100644 bindings/cxx/gpiodcxx/line-request.hpp
 create mode 100644 bindings/cxx/gpiodcxx/misc.hpp
 create mode 100644 bindings/cxx/gpiodcxx/request-config.hpp
 create mode 100644 bindings/cxx/info-event.cpp
 delete mode 100644 bindings/cxx/iter.cpp
 create mode 100644 bindings/cxx/line-config.cpp
 create mode 100644 bindings/cxx/line-info.cpp
 create mode 100644 bindings/cxx/line-request.cpp
 delete mode 100644 bindings/cxx/line.cpp
 delete mode 100644 bindings/cxx/line_bulk.cpp
 create mode 100644 bindings/cxx/misc.cpp
 create mode 100644 bindings/cxx/request-config.cpp

2.30.1

Add a getter for the edge-event buffer capacity property. This is useful if we want to read the maximum number of events that can be stored in a buffer object without externally storing its capacity.

---
 include/gpiod.h  | 9 +++++++++
 lib/edge-event.c | 6 ++++++
 2 files changed, 15 insertions(+)

index 66e6c12..8fc20ed 100644
@@ -989,6 +989,15 @@ unsigned long gpiod_edge_event_get_line_seqno(struct gpiod_edge_event *event);
 struct gpiod_edge_event_buffer *
 gpiod_edge_event_buffer_new(unsigned int capacity);
 
+/**
+ * @brief Get the capacity (the max number of events that can be stored) of
+ *        the event buffer.
+ * @param buffer Edge event buffer.
+ * @return The capacity of the buffer.
+ */
+unsigned int
+gpiod_edge_event_buffer_get_capacity(struct gpiod_edge_event_buffer *buffer);
+
 /**
  * @brief Free the edge event buffer and release all associated resources.
  * @param buffer Edge event buffer to free.

index 4aece4b..ab484ad 100644
@@ -109,6 +109,12 @@ gpiod_edge_event_buffer_new(unsigned int capacity)
 	return buf;
 }
 
+GPIOD_API unsigned int
+gpiod_edge_event_buffer_get_capacity(struct gpiod_edge_event_buffer *buffer)
+{
+	return buffer->capacity;
+}
+
 GPIOD_API void
 gpiod_edge_event_buffer_free(struct gpiod_edge_event_buffer *buffer)
 {

2.30.1

This adds new public functions for interacting with line request objects. First we implement getters for the number of lines in a request and for the array of offsets of requested lines. Next we rename the existing gpiod_line_request_get/set_values() functions to gpiod_line_request_get/set_values_subset() and add new functions that are named gpiod_line_request_get/set_values() but allow to get/set the values of all requested lines.

---
 include/gpiod.h    | 54 ++++++++++++++++++++++++++++++++++++++++------
 lib/line-request.c | 50 +++++++++++++++++++++++++++++++++---------
 tools/gpioget.c    |  3 +--
 3 files changed, 89 insertions(+), 18 deletions(-)

index 8fc20ed..51e93be 100644
@@ -814,6 +814,22 @@ gpiod_request_config_set_event_buffer_size(struct gpiod_request_config *config,
  */
 void gpiod_line_request_release(struct gpiod_line_request *request);
 
+/**
+ * @brief Get the number of lines in this request.
+ * @param request Line request object.
+ * @return Number of requested lines.
+ */
+unsigned int gpiod_line_request_get_num_lines(struct gpiod_line_request *request);
+
+/**
+ * @brief Get the hardware offsets of lines in this request.
+ * @param request Line request object.
+ * @param offsets Array to store offsets in. Must hold at least the number of
+ *                lines returned by ::gpiod_line_request_get_num_lines.
+ */
+void gpiod_line_request_get_offsets(struct gpiod_line_request *request,
+				    unsigned int *offsets);
+
 /**
  * @brief Read the value of a single line associated with this request.
  * @param request Line request object.

@@ -824,7 +840,7 @@ int gpiod_line_request_get_value(struct gpiod_line_request *request,
 				 unsigned int offset);
 
 /**
- * @brief Read values of lines associated with this request.
+ * @brief Read values of a subset of lines associated with this request.
  * @param request GPIO line request.
  * @param num_lines Number of lines for which to read values.
  * @param offsets Array of offsets corresponding with the lines associated with

@@ -832,9 +848,23 @@ int gpiod_line_request_get_value(struct gpiod_line_request *request,
  * @param values Array in which the values will be stored.
  * @return 0 on success, -1 on failure.
  */
+int gpiod_line_request_get_values_subset(struct gpiod_line_request *request,
+					 unsigned num_lines,
+					 const unsigned int *offsets,
+					 int *values);
+
+/**
+ * @brief Read values of all lines associated with this request.
+ * @param request GPIO line request.
+ * @param values Array in which the values will be stored. Must hold at least
+ *               the number of lines returned by
+ *               ::gpiod_line_request_get_num_lines. The index of each value
+ *               will be associated with the offset at the same index in the
+ *               offset array returned by ::gpiod_line_request_get_offsets.
+ * @return 0 on success, -1 on failure.
+ */
 int gpiod_line_request_get_values(struct gpiod_line_request *request,
-				  unsigned num_lines,
-				  const unsigned int *offsets, int *values);
+				  int *values);
 
 /**
  * @brief Set the value of a single line associated with this request.

@@ -846,7 +876,7 @@ int gpiod_line_request_set_value(struct gpiod_line_request *request,
 				 unsigned int offset, int value);
 
 /**
- * @brief Set values of lines associated with this line request.
+ * @brief Set values of a subset of lines associated with this line request.
  * @param request GPIO line request.
  * @param num_lines Number of lines for which to set values.
  * @param offsets Array of offsets corresponding with the lines associated with

@@ -855,9 +885,21 @@ int gpiod_line_request_set_value(struct gpiod_line_request *request,
  *               correspond with the offsets in the previous argument.
  * @return 0 on success, -1 on failure.
  */
+int gpiod_line_request_set_values_subset(struct gpiod_line_request *request,
+					 unsigned int num_lines,
+					 const unsigned int *offsets,
+					 const int *values);
+
+/**
+ * @brief Set values of all lines associated with this request.
+ * @param request GPIO line request.
+ * @param values Array containing the values to set. Must hold at least the
+ *               number of lines returned by ::gpiod_line_request_get_num_lines.
+ *               The index of each value be associated with the offset at the
+ *               same index in the offset array returned by
+ *               ::gpiod_line_request_get_offsets.
+ */
 int gpiod_line_request_set_values(struct gpiod_line_request *request,
-				  unsigned int num_lines,
-				  const unsigned int *offsets,
 				  const int *values);
 
 /**

index e0c630d..afa2ccb 100644
@@ -129,8 +129,7 @@ int main(int argc, char **argv)
 	if (!request)
 		die_perror("unable to request lines");
 
-	ret = gpiod_line_request_get_values(request, num_lines,
-					    offsets, values);
+	ret = gpiod_line_request_get_values(request, values);
 	if (ret)
 		die_perror("unable to read GPIO line values");
 

2.30.1

This is the bulk of work implementing C++ bindings for the new libgpiod API. The tests are not converted yet but the examples are fully functional. More details in the cover letter as this patch will be squashed with the one for the core C library anyway.

---
 Doxyfile.in                                 |   4 +-
 bindings/cxx/Makefile.am                    |  16 +-
 bindings/cxx/chip.cpp                       | 214 +++--
 bindings/cxx/edge-event-buffer.cpp          | 103 +++
 bindings/cxx/edge-event.cpp                 | 123 +++
 bindings/cxx/examples/Makefile.am           |  12 +-
 bindings/cxx/examples/gpiodetectcxx.cpp     |   3 +-
 bindings/cxx/examples/gpiogetcxx.cpp        |  12 +-
 bindings/cxx/examples/gpioinfocxx.cpp       |  63 +-
 bindings/cxx/examples/gpiomoncxx.cpp        |  39 +-
 bindings/cxx/examples/gpiosetcxx.cpp        |  19 +-
 bindings/cxx/gpiod.hpp                      | 938 +-------------------
 bindings/cxx/gpiodcxx/Makefile.am           |  14 +
 bindings/cxx/gpiodcxx/chip.hpp              | 180 ++++
 bindings/cxx/gpiodcxx/edge-event-buffer.hpp | 115 +++
 bindings/cxx/gpiodcxx/edge-event.hpp        | 124 +++
 bindings/cxx/gpiodcxx/info-event.hpp        | 107 +++
 bindings/cxx/gpiodcxx/line-config.hpp       | 244 +++++
 bindings/cxx/gpiodcxx/line-info.hpp         | 205 +++++
 bindings/cxx/gpiodcxx/line-request.hpp      | 207 +++++
 bindings/cxx/gpiodcxx/misc.hpp              |  49 +
 bindings/cxx/gpiodcxx/request-config.hpp    |  97 ++
 bindings/cxx/info-event.cpp                 |  89 ++
 bindings/cxx/internal.hpp                   | 168 +++-
 bindings/cxx/iter.cpp                       |  60 --
 bindings/cxx/line-config.cpp                | 226 +++++
 bindings/cxx/line-info.cpp                  | 150 ++++
 bindings/cxx/line-request.cpp               | 194 ++++
 bindings/cxx/line.cpp                       | 321 -------
 bindings/cxx/line_bulk.cpp                  | 366 --------
 bindings/cxx/misc.cpp                       |  18 +
 bindings/cxx/request-config.cpp             |  80 ++
 configure.ac                                |   1 +
 33 files changed, 2734 insertions(+), 1827 deletions(-)
 create mode 100644 bindings/cxx/edge-event-buffer.cpp
 create mode 100644 bindings/cxx/edge-event.cpp
 create mode 100644 bindings/cxx/gpiodcxx/Makefile.am
 create mode 100644 bindings/cxx/gpiodcxx/chip.hpp
 create mode 100644 bindings/cxx/gpiodcxx/edge-event-buffer.hpp
 create mode 100644 bindings/cxx/gpiodcxx/edge-event.hpp
 create mode 100644 bindings/cxx/gpiodcxx/info-event.hpp
 create mode 100644 bindings/cxx/gpiodcxx/line-config.hpp
 create mode 100644 bindings/cxx/gpiodcxx/line-info.hpp
 create mode 100644 bindings/cxx/gpiodcxx/line-request.hpp
 create mode 100644 bindings/cxx/gpiodcxx/misc.hpp
 create mode 100644 bindings/cxx/gpiodcxx/request-config.hpp
 create mode 100644 bindings/cxx/info-event.cpp
 delete mode 100644 bindings/cxx/iter.cpp
 create mode 100644 bindings/cxx/line-config.cpp
 create mode 100644 bindings/cxx/line-info.cpp
 create mode 100644 bindings/cxx/line-request.cpp
 delete mode 100644 bindings/cxx/line.cpp
 delete mode 100644 bindings/cxx/line_bulk.cpp
 create mode 100644 bindings/cxx/misc.cpp
 create mode 100644 bindings/cxx/request-config.cpp

index 22d4721..53e8c2b 100644
@@ -44,7 +44,9 @@ WARNINGS               = YES
 WARN_IF_UNDOCUMENTED   = YES
 WARN_FORMAT            =
 WARN_LOGFILE           =
-INPUT                  = @top_srcdir@/include/gpiod.h @top_srcdir@/bindings/cxx/gpiod.hpp
+INPUT                  = @top_srcdir@/include/gpiod.h \
+                         @top_srcdir@/bindings/cxx/gpiod.hpp \
+                         @top_srcdir@/bindings/cxx/gpiodcxx/
 SOURCE_BROWSER         = YES
 INLINE_SOURCES         = NO
 REFERENCED_BY_RELATION = YES

index d9fa577..4f7eb47 100644
@@ -2,8 +2,18 @@
 # SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <[email protected]>
 
 lib_LTLIBRARIES = libgpiodcxx.la
-libgpiodcxx_la_SOURCES = chip.cpp internal.h iter.cpp line.cpp line_bulk.cpp
-libgpiodcxx_la_CPPFLAGS = -Wall -Wextra -g -std=gnu++11
+libgpiodcxx_la_SOURCES = \
+	chip.cpp \
+	edge-event-buffer.cpp \
+	edge-event.cpp \
+	info-event.cpp \
+	internal.hpp \
+	line-config.cpp \
+	line-info.cpp \
+	line-request.cpp \
+	misc.cpp \
+	request-config.cpp
+libgpiodcxx_la_CPPFLAGS = -Wall -Wextra -g -std=gnu++17
 libgpiodcxx_la_CPPFLAGS += -fvisibility=hidden -I$(top_srcdir)/include/
 libgpiodcxx_la_LDFLAGS = -version-info $(subst .,:,$(ABI_CXX_VERSION))
 libgpiodcxx_la_LDFLAGS += -lgpiod -L$(top_builddir)/lib

@@ -13,7 +23,7 @@ include_HEADERS = gpiod.hpp
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = libgpiodcxx.pc
 
-SUBDIRS = .
+SUBDIRS = gpiodcxx .
 
 if WITH_TESTS
 

index 748b581..7e29616 100644
@@ -5,12 +5,12 @@ AM_CPPFLAGS = -I$(top_srcdir)/bindings/cxx/ -I$(top_srcdir)/include
 AM_CPPFLAGS += -Wall -Wextra -g -std=gnu++17
 AM_LDFLAGS = -lgpiodcxx -L$(top_builddir)/bindings/cxx/ -lstdc++fs
 
-noinst_PROGRAMS =				\
-		gpiodetectcxx			\
-		gpiofindcxx			\
-		gpiogetcxx			\
-		gpioinfocxx			\
-		gpiomoncxx			\
+noinst_PROGRAMS = \
+		gpiodetectcxx \
+		gpiofindcxx \
+		gpiogetcxx \
+		gpioinfocxx \
+		gpiomoncxx \
 		gpiosetcxx
 
 gpiodetectcxx_SOURCES = gpiodetectcxx.cpp

index 872cd96..15c5e71 100644
@@ -3,10 +3,9 @@
 
 /* C++ reimplementation of the gpiodetect tool. */
 
-#include <gpiod.hpp>
-
 #include <cstdlib>
 #include <filesystem>
+#include <gpiod.hpp>
 #include <iostream>
 
 int main(int argc, char **argv)

index 94b3dac..e9372c8 100644
@@ -15,21 +15,15 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	::std::vector<unsigned int> offsets;
+	::gpiod::line_offsets offsets;
 
 	for (int i = 2; i < argc; i++)
 		offsets.push_back(::std::stoul(argv[i]));
 
 	::gpiod::chip chip(argv[1]);
-	auto lines = chip.get_lines(offsets);
+	auto request = chip.request_lines(::gpiod::request_config(offsets, "gpiogetcxx"));
 
-	lines.request({
-		argv[0],
-		::gpiod::line_request::DIRECTION_INPUT,
-		0
-	});
-
-	auto vals = lines.get_values();
+	auto vals = request.get_values();
 
 	for (auto& it: vals)
 		::std::cout << it << ' ';

index 4d6ac6e..5e4b77e 100644
@@ -3,29 +3,27 @@
 
 /* Simplified C++ reimplementation of the gpiomon tool. */
 
-#include <gpiod.hpp>
-
+#include <chrono>
 #include <cstdlib>
+#include <gpiod.hpp>
 #include <iostream>
 
 namespace {
 
-void print_event(const ::gpiod::line_event& event)
+void print_event(const ::gpiod::edge_event& event)
 {
-	if (event.event_type == ::gpiod::line_event::RISING_EDGE)
+	if (event.event_type() == ::gpiod::edge_event::RISING_EDGE)
 		::std::cout << " RISING EDGE";
-	else if (event.event_type == ::gpiod::line_event::FALLING_EDGE)
-		::std::cout << "FALLING EDGE";
 	else
-		throw ::std::logic_error("invalid event type");
+		::std::cout << "FALLING EDGE";
 
 	::std::cout << " ";
 
-	::std::cout << ::std::chrono::duration_cast<::std::chrono::seconds>(event.timestamp).count();
+	::std::cout << event.timestamp() / 1000000000;
 	::std::cout << ".";
-	::std::cout << event.timestamp.count() % 1000000000;
+	::std::cout << event.timestamp() % 1000000000;
 
-	::std::cout << " line: " << event.source.offset();
+	::std::cout << " line: " << event.line_offset();
 
 	::std::cout << ::std::endl;
 }

@@ -39,25 +37,24 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	::std::vector<unsigned int> offsets;
+	::gpiod::line_offsets offsets;
 	offsets.reserve(argc);
 	for (int i = 2; i < argc; i++)
 		offsets.push_back(::std::stoul(argv[i]));
 
 	::gpiod::chip chip(argv[1]);
-	auto lines = chip.get_lines(offsets);
+	auto request = chip.request_lines(::gpiod::request_config(offsets, "gpiomoncxx"),
+					  ::gpiod::line_config(::gpiod::line_config::DIRECTION_INPUT,
+							       ::gpiod::line_config::EDGE_BOTH));
 
-	lines.request({
-		argv[0],
-		::gpiod::line_request::EVENT_BOTH_EDGES,
-		0,
-	});
+	::gpiod::edge_event_buffer buffer;
 
 	for (;;) {
-		auto events = lines.event_wait(::std::chrono::seconds(1));
-		if (events) {
-			for (auto& it: events)
-				print_event(it.event_read());
+		if (request.edge_event_wait(::std::chrono::seconds(5))) {
+			request.edge_event_read(buffer);
+
+			for (const auto& event: buffer)
+				print_event(event);
 		}
 	}
 

index 71b27a9..0e31253 100644
@@ -11,12 +11,13 @@
 int main(int argc, char **argv)
 {
 	if (argc < 3) {
-		::std::cerr << "usage: " << argv[0] << " <chip> <line_offset0>=<value0> ..." << ::std::endl;
+		::std::cerr << "usage: " << argv[0] <<
+			       " <chip> <line_offset0>=<value0> ..." << ::std::endl;
 		return EXIT_FAILURE;
 	}
 
-	::std::vector<unsigned int> offsets;
-	::std::vector<int> values;
+	::gpiod::line_offsets offsets;
+	::gpiod::line_values values;
 
 	for (int i = 2; i < argc; i++) {
 		::std::string arg(argv[i]);

@@ -27,20 +28,18 @@ int main(int argc, char **argv)
 		::std::string value(arg.substr(pos + 1, ::std::string::npos));
 
 		if (offset.empty() || value.empty())
-			throw ::std::invalid_argument("invalid argument: " + ::std::string(argv[i]));
+			throw ::std::invalid_argument("invalid offset=value mapping: " +
+						      ::std::string(argv[i]));
 
 		offsets.push_back(::std::stoul(offset));
 		values.push_back(::std::stoul(value));
 	}
 
 	::gpiod::chip chip(argv[1]);
-	auto lines = chip.get_lines(offsets);
+	auto request = chip.request_lines(::gpiod::request_config(offsets, "gpiosetcxx"),
+				::gpiod::line_config(::gpiod::line_config::DIRECTION_OUTPUT));
 
-	lines.request({
-		argv[0],
-		::gpiod::line_request::DIRECTION_OUTPUT,
-		0
-	}, values);
+	request.set_values(values);
 
 	::std::cin.get();
 

new file mode 100644
index 0000000..f5ef9ce
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+# SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <[email protected]>
+
+otherincludedir = $(includedir)/gpiodcxx
+otherinclude_HEADERS = \
+	chip.hpp \
+	edge-event-buffer.hpp \
+	edge-event.hpp \
+	info-event.hpp \
+	line-config.hpp \
+	line-info.hpp \
+	line-request.hpp \
+	misc.hpp \
+	request-config.hpp

new file mode 100644
index 0000000..a5639c6

index e0a917f..a0fd45a 100644

2.30.1

Part of the diff is not shown here. View the raw message.

[...]

[...]

Ben Hutchings · Senior Embedded Software Engineer, Essensium-Mind · mind.be

I'm not sure there would be any benefit to enum classes here except for longer scope when using them in code. I would prefer to leave it this way.

Bartosz

Actually after a second thought, it wouldn't make it long - it would just look like: line_info::direction::INPUT instead. Maybe it is a more C++ approach after all.

Bart

I tried to change it but if - for example - I implement the enum type for the direction property of line_info as:

enum class direction {
    INPUT = 1,
    OUTPUT
};

then it conflicts with the getter of line_info: direction(void) const.

One option is to rename all getters to get_<property>(). What do you think?

Bartosz

Hello Bartosz et al.,

Apologies for blowing hot and cold with regard to my contributions and responses on this mailing list. The last nine months have been an incredibly busy time.

I would recommend using enum classes, as this offers better type safety. Best practice is to avoid the use of unscoped enums within C++, especially of the anonymous variety. There is now a set of ISO C++ Core Guidelines (see
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#main)
which offer sage advice for developing C++ software. Just be aware that one-size doesn't fit all and trying to maintain shared library ABI compatibility would be considered an edge case which affects the applicability of some guidance. In time, maybe someone will explicitly state which guidelines are exempt in a situation similar to our own (i.e., providing ABI stable C++ interfaces to shared libraries for major revisions).

ABI compatibility issues can be avoided by using one of the fixed width integer types introduced in C++11 (e.g., std::uint8_t) as the underlying type of the scoped enum class.

I would personally avoid adding the 'get_' prefix to 'getter' methods of classes, although this choice is almost entirely aesthetical in nature.

My advice would be to remove all anonymous unscoped enum definitions from class definitions, instead defining them as scoped enum classes using a suitable fixed-width integer type (i.e., std::uint8_t) outside the dependent classes. Then, within the definitions of dependent classes, you can define publicly scoped type aliases for these enum class types (using the suffix '_type' for these aliases, as is commonly seen with other C++ libraries). See my rough example below for illustrative purposes only. This is a familiar approach and will rectify your current issue with naming collisions.

namespace gpiod {

enum class direction : std::uint8_t {
    INPUT = 1,
    OUTPUT
};

class line_info {
public:

    using direction_type = ::gpiod::direction;

    direction_type direction () const {
        return _m_direction;
    }

private:

    direction_type _m_direction;
};

}

If time permits, I'll do a proper review of your changes (however, I emigrate in less than a week, so I'm still rather busy at the moment).

Best,
Jack

Oh, nice, wish you all the best!

Is there any disadvantage to declaring the scoped class enums as subtypes within the concerned classes?

I was thinking of:

class line_info
{
public

    class direction
    {
        INPUT = 1,
        OUTPUT,
    };

    direction get_direction(void) const;
};

I also don't like get_<property> style methods but IMO

direction get_direction(void) const;

is better than

direction_type direction(void) const;

Thanks for the link, I'll try to find some time to read it.

Bartosz