frame

Merge lp://staging/~chasedouglas/frame/generic into lp://staging/frame

  1. generic
  2. Merge into trunk
Proposed by Chase Douglas
Status: Merged
Merged at revision: 54
Proposed branch: lp://staging/~chasedouglas/frame/generic
Merge into: lp://staging/frame
Diff against target: 0 lines
To merge this branch: bzr merge lp://staging/~chasedouglas/frame/generic
Reviewer Review Type Date Requested Status
Stephen M. Webb (community) Approve
Jussi Pakkanen (community) Approve
Chase Douglas (community) Needs Resubmitting
Review via email: mp+86873@code.staging.launchpad.net

Description of the change

I found out about C11's new _Generic feature. I decided to make Jussi happy and add type checking support to the utouch-frame property getters.

With these changes, we now have compile-time and runtime checks for proper data types passed in by reference. For example:

// Succeeds, returns UFStatusSuccess
UFEventType type;
status = frame_event_get_property(event, UFEventPropertyType, &type);

// Fails to compile because no event properties are of type char
char test1;
status = frame_event_get_property(event, UFEventPropertyType, &test1);

// Fails at runtime because UFDevice is not the correct type for UFEventPropertyType, return UFStatusInvalidType
UFDevice test2;
status = frame_event_get_property(event, UFEventPropertyType, &test2);

This should all be backwards compatible with correctly written code. However, runtime checking is a bit pedantic. For example, using an unsigned int instead of an int will produce an error. This also adds internal functions to the API, so the version was bumped.

Unfortunately, GCC doesn't support _Generic yet. Clang 3.0 does, but there is a libstdc++ bug that prevents Clang from compiling any code that uses std::shared_ptr. Upstream libstdc++ has the fix, but it has not been backported to gcc 4.6 yet. So, the easiest way to test is:

1. Install "clang" on Precise (11.10 and earlier have old versions of Clang that don't have _Generic)
2. Add ppa:chasedouglas/ppa (my personal ppa, which right now only includes the libstdc++ fix)
3. Configure with "clang" as the compilers: CC=clang CXX=clang ./configure --prefix=/usr --with-xi
4. Run make as normal

To post a comment you must log in.
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

Having these checks is better than not having them, but I still strongly prefer type safe proper functions that are guaranteed to not fail ever.

Documentation on _Generic on the internets is quite sparse, so I can't really comment on the implementation all that much. However there are some minor issues.

You use

#if !__has_extension(c_generic_selections) /* See frame_internal.h */

in several places. This should be checked only one (in a configure test?) and put in a config.h as HAS_GENERIC_SELECTIONS or somesuch.

Redefining compiler behaviour with #define __has_extension(x) 0 in a public header feels extremely suspicious.

There are no unit tests for the runtime failure. Can you test that a piece of code does not compile with gtest?

Revision history for this message
Chase Douglas (chasedouglas) wrote :

On 01/02/2012 03:19 AM, Jussi Pakkanen wrote:
> Documentation on _Generic on the internets is quite sparse, so I can't really comment on the implementation all that much. However there are some minor issues.

TBH, I haven't found anything better than the C11 draft from April 2011:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf. See section
6.5.1.1.

> You use
>
> #if !__has_extension(c_generic_selections) /* See frame_internal.h */
>
> in several places. This should be checked only one (in a configure test?) and put in a config.h as HAS_GENERIC_SELECTIONS or somesuch.

The check in the header file is used when building the library itself
*and* when building other projects that use utouch-frame. For example,
if utouch-frame is built with clang 3.0 and has generic selections, any
use of frame_*_get_property() within utouch-frame (if there are any)
will use the _Generic() versions of the functions. However, if
utouch-grail then is built with gcc 4.6, it will use the actual
frame_*_get_property() functions that do not have any type checking.

> Redefining compiler behaviour with #define __has_extension(x) 0 in a public header feels extremely suspicious.

Yeah, it's ugly, but I can't find any other way to do it. I've seen
others suggest this as the best way to deal with compilers that do not
define __has_extension(x) themselves.

Also, if you attempt to #undef after use, gcc will throw an error
because you are undefining what it believes to be a reserved macro due
to the leading "__".

> There are no unit tests for the runtime failure. Can you test that a piece of code does not compile with gtest?

I can add a runtime error check using gtest. I can also manually add a
compilation check that is run when "make check" is run.

Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

Checking for this without #undefs or any other trickery is relatively straightforward:

#if defined(__has_extension)
  #if __has_extension(c_generic_selections)
  #define HAS_GENERIC_SELECTIONS
  #endif
#endif

I tested this with GCC and Clang and it works perfectly. Following the old programming virtue of "only ever do anything once", this should be put at the top of the header and have everything else just use HAS_GENERIC_SELECTIONS.

Revision history for this message
Stephen M. Webb (bregma) wrote :

On 01/03/2012 05:07 AM, Jussi Pakkanen wrote:
> Checking for this without #undefs or any other trickery is relatively straightforward:
>
> #if defined(__has_extension)
> #if __has_extension(c_generic_selections)
> #define HAS_GENERIC_SELECTIONS
> #endif
> #endif
>
> I tested this with GCC and Clang and it works perfectly. Following the old programming virtue of "only ever do anything once", this should be put at the top of the header and have everything else just use HAS_GENERIC_SELECTIONS.

Just as an aside, this _is_ what autoconf was invented for.

--
Stephen M. Webb <email address hidden>
Canonical Ltd.

lp://staging/~chasedouglas/frame/generic updated
59. By Chase Douglas

Fix incorrect typing in test device code

60. By Chase Douglas

Define macro if C generic selections are available

This prevents pollution of the reserved macro namespace by not defining
__has_extension(x).

61. By Chase Douglas

Fix type casting of test fixture values

Caught by compilation with clang.

62. By Chase Douglas

Add static compilation test for type checking

This checks if type checking of impossible types passed to
frame_*_get_property() fails to compile where C generic selections are
available.

63. By Chase Douglas

Add dynamic type checking test

Unfortunately, at this time the test cannot be executed because clang 3.0
cannot compile gtest derived testcases without errors, and gcc does not
support C generic selections yet. Hopefully when the compilers are better
supported things will just work.

Revision history for this message
Chase Douglas (chasedouglas) wrote :

> Checking for this without #undefs or any other trickery is relatively
> straightforward:
>
> #if defined(__has_extension)
> #if __has_extension(c_generic_selections)
> #define HAS_GENERIC_SELECTIONS
> #endif
> #endif
>
> I tested this with GCC and Clang and it works perfectly. Following the old
> programming virtue of "only ever do anything once", this should be put at the
> top of the header and have everything else just use HAS_GENERIC_SELECTIONS.

I had tried:

#if defined(__has_extension) && __has_extension(c_generic_selections)

but it didn't work. I guess CPP doesn't support short-circuited boolean tests like that. I changed everything to your suggestion. Thanks for the tip!

I have added a static compilation test. I also added a runtime dynamic typing test, but clang fails to compile gtest derived testcases, so it can't actually be tested. It is compiled and run when using g++, but it is essentially a big noop because there's no support for C generic selections.

review: Needs Resubmitting
lp://staging/~chasedouglas/frame/generic updated
64. By Chase Douglas

Ship utouch/frame_internal.h

65. By Chase Douglas

Only link libstdc++ explicitly for clang

The C++ standard says the c++ runtime should be available automatically,
so clang is "wrong". Thus, link libstdc++ only when using clang.

Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

Go.

review: Approve
Revision history for this message
Stephen M. Webb (bregma) wrote :

I'm OK with this (after offline discussion).

review: Approve
lp://staging/~chasedouglas/frame/generic updated
66. By Chase Douglas

Check for __clang__ declaration instead of relying on program name

Preview Diff

Empty

Subscribers

People subscribed via source and target branches