Hi Peter,
(+Paolo / Eric)
On 29/1/26 21:31, Peter Maydell wrote:
On Thu, 29 Jan 2026 at 20:26, Roman Kiryanov <[email protected]> wrote:
Hi Philippe, thank you for looking into my patch.
On Thu, Jan 29, 2026 at 11:23 AM Philippe Mathieu-Daudé
<[email protected]> wrote:
On 29/1/26 20:04, Roman Kiryanov wrote:
This patch fixes the C++ compilation error:
ISO C++ forbids forward references to 'enum' types
-typedef enum QKbdModifier QKbdModifier;
-
-enum QKbdModifier {
+typedef enum QKbdModifier {
-};
+} QKbdModifier;
Yes but:
$ git grep -E 'typedef enum.*;'
include/hw/misc/auxbus.h:32:typedef enum AUXCommand AUXCommand;
I also grepped through the QEMU source code and saw other examples. I
limited this patch to ui/kbd-state.h because it is the specific header
causing C++ compilation errors in our project. I hesitated to touch
the others to keep the diff minimal and avoid changes in files we
aren't currently using.
Currently, QEMU has a mix of incompatible and compatible (e.g.
block/block-common.h, BlockZoneOp) enum definitions. I think,
everything outside the include/ directory should be implementation
details.
How would you prefer I proceed?
1) Keep this patch focused: Limit the change to kbd-state.h to fix the
active build breakage.
2) Fix public headers: I can send a v2 that fixes this pattern for all
files in include/.
3) Tree-wide cleanup: I can attempt to fix every instance found by
grep, though this touches private headers as well.
There is also option 4:
4) Don't change this, because QEMU is not a C++ project
and it's not a goal for our headers to be compilable
with C++, with some minor exceptions.
What are we trying to achieve here, and why does it benefit
us as an upstream project ?
cf previous email thread from 2024
https://lore.kernel.org/qemu-devel/[email protected]/
about "drip-feeding" of this kind of patch with no clear take
on what the end state is or how much churn it's going to induce.
I clearly know QEMU is not a C++ project. Now I don't see why we should
be reluctant to have headers being usable by a C++ compiler, as long as
it doesn't make our project worst to maintain.
See for example non-invasive commit 7246c4cc470 ("exec: don't use void*
in pointer arithmetic in headers"):
-- >8 --
diff --git a/include/exec/memory.h b/include/exec/memory.h
@@ -2767 +2767 @@ struct MemoryRegionCache {
- void *ptr;
+ uint8_t *ptr;
---
or commit 17c7df806b3 ("exec: avoid using C++ keywords in function
parameters"):
-- >8 --
diff --git a/include/exec/memory.h b/include/exec/memory.h
@@ -928 +928 @@ struct MemoryListener {
- int old, int new);
+ int old_val, int new_val);
@@ -947 +947 @@ struct MemoryListener {
- int old, int new);
+ int old_val, int new_val);
---
In this particular case, I always have been confused about what would be
the size of forward-declared enums. The C99 standard chapter §6.7.2.2
point 4 mentions:
Each enumerated type shall be compatible with char, a signed
integer type, or an unsigned integer type. The choice of type
is implementation-defined, but shall be capable of representing
the values of all the members of the enumeration.
When compiling this file with -Werror=pedantic we get:
In file included from ../../ui/kbd-state.c:10:
include/ui/kbd-state.h:12:14: error: ISO C forbids forward references to
'enum' types [-Werror,-Wpedantic]
12 | typedef enum QKbdModifier QKbdModifier;
| ^
So arguably this could be fixed for C.
On another perspective, having more contributors should keep our project
healthy, and would potentially bring us more reviewers (maintainers?).
If we don't object to take such patches, my only request is to have a
CI job for non-C++ developers to not breaking the headers over time.
My 2 cents :)
Regards,
Phil.
