On Sun, Nov 18, 2012 at 12:01 PM, Nick Wellnhofer <[email protected]> wrote: > I created a branch 'c99-types' which makes the whole Lucy codebase switch to > several standard macros defined in the C99 standard. This includes > > * Integer min/max macros from stdint.h > CHY_I32_MAX => INT32_MAX > * Integer literal macros from stdint.h > CHY_I32_C => INT32_C > * printf type specifiers from inttypes.h > I64P => PRId64 > > If any of the standard headers isn't found, the macros will be defined by > Charmonizer.
Nick++ I've checked out the branch and everything looks solid. Extra kudos for that big bool_t-to-bool commit, which I imagine was tedious to prepare! One original design goal of the Charmonizer integer types was to eliminate the possibility of namespace conflicts with either system headers or arbitrary libraries. Whenever non-standard headers might be include (for example in our XS binding code which pulls in perl.h and friends) the full names for Charmonizer-created types would be in effect: e.g. `chy_i32_t`. When we could be certain that only C standard headers would be in effect (e.g. in most of the Lucy core code[1]), we could use "short names" like i32_t. The short names were still guaranteed not to conflict with anything, since only the symbols from standard C would be competing and those are a known, finite set. Other projects define similar types -- e.g. APR has apr_int32_t: <https://apr.apache.org/docs/apr/trunk/group__apr__platform.html>. Switching over to stdint.h names for most integer types -- e.g. from chy_i32_t/i32_t to int32_t -- means that we no longer have to explain our conventions to new contributors coming up to speed. However, it also means that we're no longer hermetically sealed off in our own namespace -- we're futzing around in the same standard namespace with everybody else, and we have to deal with both incomplete standard environments and with other people's attempts to "fix" the standard environments which may conflict with our own. It's sure been nice having our own 100% reliable boolean type for this long without having to deal with the standards mess! >From the perspective of Charmonizer as a component in isolation (which has the potential to spin off from Lucy one day) I wonder whether it wouldn't be advantageous to leave intrusions into the standard namespace up to the consumer: #ifndef CHY_HAS_STDINT_H #define int32_t chy_i32_t ... #endif >From the perspective of Lucy and Clownfish, though, exactly where the names get defined is an implementation detail, so long as they are still defined in a common header -- what matters more is that the codebase has migrated towards C99. Still, I'm a little uneasy about whether Clownfish ought to support all of C's fifty bajillion integer types. On one hand, I would love it if we could kill all of them save for a single 64-bit integer primitive type and then normalize all arguments to Clownfish methods to 64 bits. On the other hand, Clownfish is a low-level library and so there's an argument to be made that it should support all system types. (We already support va_list, int, and size_t because it was hard to do without them.) > On top of that, I tested switching from 'chy_bool_t' to 'bool' from > stdbool.h (or from the C++ compiler when compiling as C++). This is a bit > problematic since 'bool' is often defined by other headers. The fact that we require either C99 or C++ for Lucy and Clownfish ought to make things easier because we are guaranteed to have either stdbool.h or the C++ `bool` type available. Our `bool` is guaranteed to come from the system one way or another. Since Charmonizer only requires C89, it's not possible for Charmonizer to resolve this problem in the general case. If you want to fake up a bool type for C89, you need to `typedef bool` to something -- `char` and `int` are both popular -- and there's significant potential for conflict with someone who has made a similar but incompatible hack. My understanding is that the stdbool.h `bool` and the C++ `bool` are not technically compatible, but I hope that won't matter in practice. Hopefully no system vendor will define them to be different sizes. > Unfortunately, also the Perl headers define their own version of 'bool' > under certain conditions. As a work-around, I had to add -DHAS_BOOL to the > build options to make the Linux and Windows build work. This looks a bit > sketchy and might cause problems with other Perl versions. The workaround doesn't seem any sketchier to me than the fact that Perl defines its own `bool` in the first place. After your changes, our `bool` is either a macro defined in stdbool.h or a C++ builtin. It's not a careless hack. * If the Perl headers are conflicting with stdbool.h, that's messed up -- maybe it happens because Perl wants to use the keyword `bool`, but also wants to specify `-std=c89`? * If the Perl headers are conflicting with C++, that's more understandable. But if the Perl devs have seen fit to provide a flag which enables C++ compatibility, let's take advantage of it. > I successfully tested: > > - MacOS 10.8 with stock Perl 5.12 (64-bit) > - Debian sid with stock Perl 5.14 (32-bit) > - Windows 7 with ActivePerl 5.16 and MSVC6 (32-bit) Nice! Marvin Humphrey [1] Except for places like FSFileHandle.c which need e.g. windows.h or other OS-specific functionality.
