Bug 121435 – Invalid code generated for clang

• Component: Bindings
• Status: RESOLVED INVALID

[Allan Sandfeld Jensen]

In the generation code we currently append a space to the variable $implType, but $implType is later used in compile assert to construct method names where it is assumed it does not have trailing white-space.

Since we require C++11 already, the appended space can be removed. It was only appended to avoid template<S<T>> syntax.

[Allan Sandfeld Jensen]

Created attachment 211792 [details]
Patch

[Anders Carlsson]

(In reply to comment #0)
> In the generation code we currently append a space to the variable $implType, but $implType is later used in compile assert to construct method names where it is assumed it does not have trailing white-space.

I don't know what you mean by "invalid code"? Invalid C++ code or invalid machine code? Why are we not seeing this on the Mac bots that already build with clang?

Can you give a more concrete example?

[Anders Carlsson]

Comment on attachment 211792 [details]
Patch

r- for now.

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (In reply to comment #0)
> > In the generation code we currently append a space to the variable $implType, but $implType is later used in compile assert to construct method names where it is assumed it does not have trailing white-space.
> 
> I don't know what you mean by "invalid code"? Invalid C++ code or invalid machine code? Why are we not seeing this on the Mac bots that already build with clang?
> 
> Can you give a more concrete example?

COMPILE_ASSERT(__is_polymorphic(SVGPathSegListPropertyTearOff ), SVGPathSegListPropertyTearOff _is_not_polymorphic);

Notice the space between SVGPathSegListPropertyTearOff and _is_not_polymorphic.

[Anders Carlsson]

(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #0)
> > > In the generation code we currently append a space to the variable $implType, but $implType is later used in compile assert to construct method names where it is assumed it does not have trailing white-space.
> > 
> > I don't know what you mean by "invalid code"? Invalid C++ code or invalid machine code? Why are we not seeing this on the Mac bots that already build with clang?
> > 
> > Can you give a more concrete example?
> 
> COMPILE_ASSERT(__is_polymorphic(SVGPathSegListPropertyTearOff ), SVGPathSegListPropertyTearOff _is_not_polymorphic);
> 
> Notice the space between SVGPathSegListPropertyTearOff and _is_not_polymorphic.

I still don’t see what the problem is - COMPILE_ASSERT is being #defined as static_assert, so the above assertion just turns into:

static_assert((__is_polymorphic(SVGPathSegListPropertyTearOff )), "SVGPathSegListPropertyTearOff _is_not_polymorphic");

I think the above compile assertion should just be converted into a static_assert that uses std::is_polymorphic.

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (In reply to comment #0)
> > > > In the generation code we currently append a space to the variable $implType, but $implType is later used in compile assert to construct method names where it is assumed it does not have trailing white-space.
> > > 
> > > I don't know what you mean by "invalid code"? Invalid C++ code or invalid machine code? Why are we not seeing this on the Mac bots that already build with clang?
> > > 
> > > Can you give a more concrete example?
> > 
> > COMPILE_ASSERT(__is_polymorphic(SVGPathSegListPropertyTearOff ), SVGPathSegListPropertyTearOff _is_not_polymorphic);
> > 
> > Notice the space between SVGPathSegListPropertyTearOff and _is_not_polymorphic.
> 
> I still don’t see what the problem is - COMPILE_ASSERT is being #defined as static_assert, so the above assertion just turns into:
> 
> static_assert((__is_polymorphic(SVGPathSegListPropertyTearOff )), "SVGPathSegListPropertyTearOff _is_not_polymorphic");
> 
> I think the above compile assertion should just be converted into a static_assert that uses std::is_polymorphic.

Unless you use clang 3.0 is which case it becomes  

typedef int dummySVGPathSegListPropertyTearOff _is_not_polymorphic [(exp) ? 1 : -1]

[Allan Sandfeld Jensen]

Apparently only triggered with clang versions not supported in trunk. Sorry.

[Anders Carlsson]

(In reply to comment #7)
> Apparently only triggered with clang versions not supported in trunk. Sorry.

Yeah in trunk you would have gotten a compile error due to:

#if !COMPILER_SUPPORTS(CXX_STATIC_ASSERT)
#error "Please use a compiler that supports C++11 static_assert."
#endif