Re: [xkbcommon] Use an integer type for modifiers bit mask.
2013/10/4 Thiago Macieira thiago.macie...@intel.com: 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.
Hi, On 4 October 2013 13:09, Wander Lairson Costa wander.lair...@gmail.com 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.
Hi On Fri, Oct 4, 2013 at 2:34 PM, Daniel Stone dan...@fooishbar.org wrote: On 4 October 2013 13:09, Wander Lairson Costa wander.lair...@gmail.com 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 14:06, Wander Lairson Costa wander.lair...@gmail.com wrote: 2013/10/4 Daniel Stone dan...@fooishbar.org: 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.
2013/10/4 David Herrmann dh.herrm...@gmail.com: Hi On Fri, Oct 4, 2013 at 2:34 PM, Daniel Stone dan...@fooishbar.org wrote: On 4 October 2013 13:09, Wander Lairson Costa wander.lair...@gmail.com 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.
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 dan...@fooishbar.org wrote: On 4 October 2013 13:09, Wander Lairson Costa wander.lair...@gmail.com 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.
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.
2013/10/4 Thiago Macieira thiago.macie...@intel.com: 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.
#include iostream
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 thiago.macie...@intel.com:
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
