Re: [xkbcommon] Use an integer type for modifiers bit mask.
#include enum Enum {ZERO, A, B, C, D}; Enum operator|(Enum a, Enum b) { return Enum(int(a)|int(b)); } int main() { Enum a(A); Enum b(B); Enum c = a|b; std::cout << a << ',' << b << ',' << c << std::endl; } Wander Lairson Costa wrote: 2013/10/4 Thiago Macieira : On sexta-feira, 4 de outubro de 2013 09:09:32, Wander Lairson Costa wrote: The issued raised when I took code from window.c in the weston clients: mask = xkb_state_serialize_mods(input->xkb.state, XKB_STATE_DEPRESSED | XKB_STATE_LATCHED); g++ gave me a build error because I was passing an integer to enum parameter. C++ is a bit more nit-picky than C regarding implicit conversions. Therefore I had to use a cast. Like you said, it's window.c. Why are you copy & pasting C code into a C++ file? Well, weston clients are the only code reference I had in hand. Sometimes I go to qtwayland also, but it did not run away from the casts... With the ABI that GCC uses, it's always at least 4 bytes. Personally, I don't like enum's in the ABI at all, because according to C/C++ standards, the compiler is free to choose whatever type it likes, and indeed I had problems with that in the past. C++11 fixed that [1]. But nevermind. We're not relying on the ABI. I said "The enum must be backed by an integer with at least as many bits as the enum possesses.". If you cast back from an int that contains one of the enum values or an OR combination of enum values, it *will* fit. I think I caused more confusion than explained here. My original point was not if the value fits or not, but that once you do whatever math with the enum, the result is not an enum type anymore (yes, sounds nit-picky)... ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
2013/10/4 Thiago Macieira : > On sexta-feira, 4 de outubro de 2013 09:09:32, Wander Lairson Costa wrote: >> The issued raised when I took code from window.c in the weston clients: >> >> mask = xkb_state_serialize_mods(input->xkb.state, >> XKB_STATE_DEPRESSED | >> XKB_STATE_LATCHED); >> >> g++ gave me a build error because I was passing an integer to enum >> parameter. C++ is a bit more nit-picky than C regarding implicit >> conversions. Therefore I had to use a cast. > > Like you said, it's window.c. Why are you copy & pasting C code into a C++ > file? > Well, weston clients are the only code reference I had in hand. Sometimes I go to qtwayland also, but it did not run away from the casts... >> > With the ABI that GCC uses, it's always at least 4 bytes. >> >> Personally, I don't like enum's in the ABI at all, because according >> to C/C++ standards, the compiler is free to choose whatever type it >> likes, and indeed I had problems with that in the past. C++11 fixed >> that [1]. But nevermind. > > We're not relying on the ABI. I said "The enum must be backed by an integer > with at least as many bits as the enum possesses.". If you cast back from an > int that contains one of the enum values or an OR combination of enum values, > it *will* fit. I think I caused more confusion than explained here. My original point was not if the value fits or not, but that once you do whatever math with the enum, the result is not an enum type anymore (yes, sounds nit-picky)... -- Best Regards, Wander Lairson Costa ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
On sexta-feira, 4 de outubro de 2013 09:09:32, Wander Lairson Costa wrote: > The issued raised when I took code from window.c in the weston clients: > > mask = xkb_state_serialize_mods(input->xkb.state, > XKB_STATE_DEPRESSED | > XKB_STATE_LATCHED); > > g++ gave me a build error because I was passing an integer to enum > parameter. C++ is a bit more nit-picky than C regarding implicit > conversions. Therefore I had to use a cast. Like you said, it's window.c. Why are you copy & pasting C code into a C++ file? > > With the ABI that GCC uses, it's always at least 4 bytes. > > Personally, I don't like enum's in the ABI at all, because according > to C/C++ standards, the compiler is free to choose whatever type it > likes, and indeed I had problems with that in the past. C++11 fixed > that [1]. But nevermind. We're not relying on the ABI. I said "The enum must be backed by an integer with at least as many bits as the enum possesses.". If you cast back from an int that contains one of the enum values or an OR combination of enum values, it *will* fit. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center signature.asc Description: This is a digitally signed message part. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
On Fri, Oct 04, 2013 at 03:22:59PM +0200, David Herrmann wrote: > Hi > > On Fri, Oct 4, 2013 at 2:34 PM, Daniel Stone wrote: > > On 4 October 2013 13:09, Wander Lairson Costa > > wrote: > >> That's what the patch is about: avoid casts. Whenever you use a cast, > >> you are giving up the help the compiler may give to you regarding > >> invalid type conversions. So, I always use the rule of thumb of > >> avoiding casts whenever I can. IMO, this is not a harmful patch and > >> will make the C++ programmers a little bit easier. > > > > I can see where it's going, but on the other hand xkb_mod_mask_t is > > _so_ not the right type. That's a bitmask of actual modifiers (i.e. > > the return value), not a bitmask of components. So it avoids the > > cast, but also makes the API look extremely misleading, as we've > > documented exactly what xkb_mod_mask_t is. > > > > I really like the type safety aspect, so to be honest I'm just > > inclined to make C++ wear the cast for now. > > We had a similar discussion when I pointed out that "enum" may change > the underlying type without notice when adding new enum-values. Just adding a note that this problem can be mitigated by adding some dummy value to the enum to force it to be int-sized. Of course we can't do it in xkbcommon, but future APIs may want to do this, at least if they don't find it too ugly. Ran ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
2013/10/4 David Herrmann : > Hi > > On Fri, Oct 4, 2013 at 2:34 PM, Daniel Stone wrote: >> On 4 October 2013 13:09, Wander Lairson Costa >> wrote: >>> That's what the patch is about: avoid casts. Whenever you use a cast, >>> you are giving up the help the compiler may give to you regarding >>> invalid type conversions. So, I always use the rule of thumb of >>> avoiding casts whenever I can. IMO, this is not a harmful patch and >>> will make the C++ programmers a little bit easier. >> >> I can see where it's going, but on the other hand xkb_mod_mask_t is >> _so_ not the right type. That's a bitmask of actual modifiers (i.e. >> the return value), not a bitmask of components. So it avoids the >> cast, but also makes the API look extremely misleading, as we've >> documented exactly what xkb_mod_mask_t is. >> >> I really like the type safety aspect, so to be honest I'm just >> inclined to make C++ wear the cast for now. > > We had a similar discussion when I pointed out that "enum" may change > the underlying type without notice when adding new enum-values. But I > agree with Daniel, the GCC type-safety for enums in C is a very handy > feature. So I'm also no big fan of the patch, sorry. > We have conflictings pros and cons here, with different tradeoffs, I perfectly understand that. -- Best Regards, Wander Lairson Costa ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
Hi, On 4 October 2013 14:06, Wander Lairson Costa wrote: > 2013/10/4 Daniel Stone : >> Well sure, but we took the added benefit of type safety (at least with >> C compilers) over the the small potential downside. I was fully aware >> of the technical point there, but don't see it as much of a risk at >> all. > > The problem can arise when you compile the lib with one compiler, and > try to use it with another one. The was exactly the problem I had. I > was responsible for an USB library that, on Windows, I used to build > with Visual C++, but a client was using C++ Builder. Couple of hours > with the disassembly and I discovered that Visual C++ chose a 32 bits > types to represent the enum, but C++ Builder chose a 8 bits type... Yeah, I'm aware of the risk ... luckily we target systems with more coherent toolchains. ;) But still, we'll cross that bridge when we come to it. >> I really like the type safety aspect, so to be honest I'm just >> inclined to make C++ wear the cast for now. > > If you change your mind, I can resubmit the patch with the integer type fixed > :) Well, we'd just be creating integer types and using them everywhere. enum/integer conversions in C++ are not always cast-free either, and all it means is that we'd be throwing away the type safety we have today, and still not necessarily getting rid of the casts. The reason I went for this is that the names can be confusing, and I'd like to warn people very visibly and immediately when they get something wrong. Demoting everything to an integer removes that, so I'd like to see the users who are hitting this just deal with the cast. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
Hi On Fri, Oct 4, 2013 at 2:34 PM, Daniel Stone wrote: > On 4 October 2013 13:09, Wander Lairson Costa > wrote: >> That's what the patch is about: avoid casts. Whenever you use a cast, >> you are giving up the help the compiler may give to you regarding >> invalid type conversions. So, I always use the rule of thumb of >> avoiding casts whenever I can. IMO, this is not a harmful patch and >> will make the C++ programmers a little bit easier. > > I can see where it's going, but on the other hand xkb_mod_mask_t is > _so_ not the right type. That's a bitmask of actual modifiers (i.e. > the return value), not a bitmask of components. So it avoids the > cast, but also makes the API look extremely misleading, as we've > documented exactly what xkb_mod_mask_t is. > > I really like the type safety aspect, so to be honest I'm just > inclined to make C++ wear the cast for now. We had a similar discussion when I pointed out that "enum" may change the underlying type without notice when adding new enum-values. But I agree with Daniel, the GCC type-safety for enums in C is a very handy feature. So I'm also no big fan of the patch, sorry. Thanks David ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
Hi, On 4 October 2013 13:09, Wander Lairson Costa wrote: > The issued raised when I took code from window.c in the weston clients: > > mask = xkb_state_serialize_mods(input->xkb.state, > XKB_STATE_DEPRESSED | > XKB_STATE_LATCHED); > > g++ gave me a build error because I was passing an integer to enum > parameter. C++ is a bit more nit-picky than C regarding implicit > conversions. Therefore I had to use a cast. One of so very many reasons I hate C++. But, unfortunately in the real world, we have to support people building with C++, so ... >> With the ABI that GCC uses, it's always at least 4 bytes. > > Personally, I don't like enum's in the ABI at all, because according > to C/C++ standards, the compiler is free to choose whatever type it > likes, and indeed I had problems with that in the past. C++11 fixed > that [1]. But nevermind. Well sure, but we took the added benefit of type safety (at least with C compilers) over the the small potential downside. I was fully aware of the technical point there, but don't see it as much of a risk at all. >> The only thing is that you need to cast it from integer back to the enum >> type. > > That's what the patch is about: avoid casts. Whenever you use a cast, > you are giving up the help the compiler may give to you regarding > invalid type conversions. So, I always use the rule of thumb of > avoiding casts whenever I can. IMO, this is not a harmful patch and > will make the C++ programmers a little bit easier. I can see where it's going, but on the other hand xkb_mod_mask_t is _so_ not the right type. That's a bitmask of actual modifiers (i.e. the return value), not a bitmask of components. So it avoids the cast, but also makes the API look extremely misleading, as we've documented exactly what xkb_mod_mask_t is. I really like the type safety aspect, so to be honest I'm just inclined to make C++ wear the cast for now. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
2013/10/4 Thiago Macieira : > On quinta-feira, 3 de outubro de 2013 23:03:44, Wander Lairson Costa wrote: >> If we combine two enum values, the result is not a valid enum value >> anymore, so it cannot be attributed to an enum variable. >> >> C++ compilers will complain if such an assigment is done. > > This is done all the time in C++. The enum must be backed by an integer with > at least as many bits as the enum possesses. > The issued raised when I took code from window.c in the weston clients: mask = xkb_state_serialize_mods(input->xkb.state, XKB_STATE_DEPRESSED | XKB_STATE_LATCHED); g++ gave me a build error because I was passing an integer to enum parameter. C++ is a bit more nit-picky than C regarding implicit conversions. Therefore I had to use a cast. > With the ABI that GCC uses, it's always at least 4 bytes. Personally, I don't like enum's in the ABI at all, because according to C/C++ standards, the compiler is free to choose whatever type it likes, and indeed I had problems with that in the past. C++11 fixed that [1]. But nevermind. > The only thing is that you need to cast it from integer back to the enum type. > That's what the patch is about: avoid casts. Whenever you use a cast, you are giving up the help the compiler may give to you regarding invalid type conversions. So, I always use the rule of thumb of avoiding casts whenever I can. IMO, this is not a harmful patch and will make the C++ programmers a little bit easier. Ofcourse the libxkbcommon maintainers may have another view and reject the patch. [1] http://www.cprogramming.com/c++11/c++11-nullptr-strongly-typed-enum-class.html -- Best Regards, Wander Lairson Costa ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [xkbcommon] Use an integer type for modifiers bit mask.
On quinta-feira, 3 de outubro de 2013 23:03:44, Wander Lairson Costa wrote: > If we combine two enum values, the result is not a valid enum value > anymore, so it cannot be attributed to an enum variable. > > C++ compilers will complain if such an assigment is done. This is done all the time in C++. The enum must be backed by an integer with at least as many bits as the enum possesses. With the ABI that GCC uses, it's always at least 4 bytes. The only thing is that you need to cast it from integer back to the enum type. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel Open Source Technology Center signature.asc Description: This is a digitally signed message part. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel