Re: [Mesa-dev] [PATCH 3/8] glapi/check_table: Remove 'extern "C"' block

[Dylan Baker]

Quoting Emil Velikov (2017-12-05 07:36:25)
> On 4 December 2017 at 23:57, Dylan Baker  wrote:
> > Quoting Emil Velikov (2017-11-23 11:04:34)
> >> On 20 November 2017 at 23:12, Dylan Baker  wrote:
> >> > This doesn't actually accomplish what it's meant to do, as extern C
> >> > doesn't undefine __cplusplus, so the included headers define a template
> >> > (because __cplusplus is defined), but then that code is in an 'extern
> >> > "C"' block, and explosion.
> >> >
> >> The commit does sound a bit off, admittedly I don't fully grok what
> >> you're trying to say.
> >>
> >> The extern "C" { } construct annotates anything within as if it were a
> >> normal C code. Thus functions will have the C linkage (otherwise
> >> they'll have the C++ mangling) and structs are using the C rules.
> >> So if you have a C++ header further down the chain it will be
> >> considered as C and hell will break loose.
> >>
> >> This patch is correct, just sent a fix for glapitable.h, while glapi.h
> >> was handled with d38839a498b38b745f5bdf6b96fbafa2792670be.
> >>
> >> As-is "struct _glapi_table" will be interpret as C++ one, which may
> >> not have the same guarantees about sizeof and pointer arithmetic that
> >> we're using in the test.
> >>
> >> Hope the above provides some inspiration towards a better commit message.
> >>
> >> Thanks
> >> Emil
> >
> > So here's the problem I run into:
> >
> > In file included from ../include/c99_compat.h:28:0,
> >  from ../src/util/macros.h:29,
> >  from ../src/mapi/glapi/glapi.h:47,
> >  from ../src/mapi/glapi/tests/check_table.cpp:28:
> > ../include/no_extern_c.h:47:1: error: template with C linkage
> >  template class _IncludeInsideExternCNotPortable;
> >  ^~~~
> > [54/93] Compiling C++ object 'src/gtest/gtest@sta/src_gtest-all.cc.o'.
> >
> > So, my commit message still makes sense to me, since you can't put a 
> > template in
> > an extern "C" block, and that template is guarded by __cplusplus, which is 
> > still
> > defined even in an extern "C" block.
> >
> This is one symptom, of the issue described before. The code in
> c99_compat.h was explicitly added by Jose to make the issue obvious.
> 
> > Do you have a commit message in mind that is better?
> 
> Feel free to reuse as much or as little of my explanation. Borrowing
> from analogous fixes also works ;-)
> Some examples:
> 237dcb4aa7c39c59bfd225ae3d73caf709be216d
> a177a13033cf9356eb26e8757055037a54268a18
> 36ad8265489fc66ab45b9b74dafa353a93bdb03b
> 
> I'm going through 4/8 - seems like there's a bunch of other bugs around :-\
> Feel free to push the remainder of the series with the nitpicks addressed.
> 
> Thanks
> Emil

The APPLE extensions one?


signature.asc
Description: signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/8] glapi/check_table: Remove 'extern "C"' block

[Emil Velikov]

On 4 December 2017 at 23:57, Dylan Baker  wrote:
> Quoting Emil Velikov (2017-11-23 11:04:34)
>> On 20 November 2017 at 23:12, Dylan Baker  wrote:
>> > This doesn't actually accomplish what it's meant to do, as extern C
>> > doesn't undefine __cplusplus, so the included headers define a template
>> > (because __cplusplus is defined), but then that code is in an 'extern
>> > "C"' block, and explosion.
>> >
>> The commit does sound a bit off, admittedly I don't fully grok what
>> you're trying to say.
>>
>> The extern "C" { } construct annotates anything within as if it were a
>> normal C code. Thus functions will have the C linkage (otherwise
>> they'll have the C++ mangling) and structs are using the C rules.
>> So if you have a C++ header further down the chain it will be
>> considered as C and hell will break loose.
>>
>> This patch is correct, just sent a fix for glapitable.h, while glapi.h
>> was handled with d38839a498b38b745f5bdf6b96fbafa2792670be.
>>
>> As-is "struct _glapi_table" will be interpret as C++ one, which may
>> not have the same guarantees about sizeof and pointer arithmetic that
>> we're using in the test.
>>
>> Hope the above provides some inspiration towards a better commit message.
>>
>> Thanks
>> Emil
>
> So here's the problem I run into:
>
> In file included from ../include/c99_compat.h:28:0,
>  from ../src/util/macros.h:29,
>  from ../src/mapi/glapi/glapi.h:47,
>  from ../src/mapi/glapi/tests/check_table.cpp:28:
> ../include/no_extern_c.h:47:1: error: template with C linkage
>  template class _IncludeInsideExternCNotPortable;
>  ^~~~
> [54/93] Compiling C++ object 'src/gtest/gtest@sta/src_gtest-all.cc.o'.
>
> So, my commit message still makes sense to me, since you can't put a template 
> in
> an extern "C" block, and that template is guarded by __cplusplus, which is 
> still
> defined even in an extern "C" block.
>
This is one symptom, of the issue described before. The code in
c99_compat.h was explicitly added by Jose to make the issue obvious.

> Do you have a commit message in mind that is better?

Feel free to reuse as much or as little of my explanation. Borrowing
from analogous fixes also works ;-)
Some examples:
237dcb4aa7c39c59bfd225ae3d73caf709be216d
a177a13033cf9356eb26e8757055037a54268a18
36ad8265489fc66ab45b9b74dafa353a93bdb03b

I'm going through 4/8 - seems like there's a bunch of other bugs around :-\
Feel free to push the remainder of the series with the nitpicks addressed.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/8] glapi/check_table: Remove 'extern "C"' block

[Dylan Baker]

Quoting Emil Velikov (2017-11-23 11:04:34)
> On 20 November 2017 at 23:12, Dylan Baker  wrote:
> > This doesn't actually accomplish what it's meant to do, as extern C
> > doesn't undefine __cplusplus, so the included headers define a template
> > (because __cplusplus is defined), but then that code is in an 'extern
> > "C"' block, and explosion.
> >
> The commit does sound a bit off, admittedly I don't fully grok what
> you're trying to say.
> 
> The extern "C" { } construct annotates anything within as if it were a
> normal C code. Thus functions will have the C linkage (otherwise
> they'll have the C++ mangling) and structs are using the C rules.
> So if you have a C++ header further down the chain it will be
> considered as C and hell will break loose.
> 
> This patch is correct, just sent a fix for glapitable.h, while glapi.h
> was handled with d38839a498b38b745f5bdf6b96fbafa2792670be.
> 
> As-is "struct _glapi_table" will be interpret as C++ one, which may
> not have the same guarantees about sizeof and pointer arithmetic that
> we're using in the test.
> 
> Hope the above provides some inspiration towards a better commit message.
> 
> Thanks
> Emil

So here's the problem I run into:

In file included from ../include/c99_compat.h:28:0,
 from ../src/util/macros.h:29,
 from ../src/mapi/glapi/glapi.h:47,
 from ../src/mapi/glapi/tests/check_table.cpp:28:
../include/no_extern_c.h:47:1: error: template with C linkage
 template class _IncludeInsideExternCNotPortable;
 ^~~~
[54/93] Compiling C++ object 'src/gtest/gtest@sta/src_gtest-all.cc.o'.

So, my commit message still makes sense to me, since you can't put a template in
an extern "C" block, and that template is guarded by __cplusplus, which is still
defined even in an extern "C" block.

Do you have a commit message in mind that is better?


signature.asc
Description: signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/8] glapi/check_table: Remove 'extern "C"' block

[Dylan Baker]

Quoting Emil Velikov (2017-11-23 11:04:34)
> On 20 November 2017 at 23:12, Dylan Baker  wrote:
> > This doesn't actually accomplish what it's meant to do, as extern C
> > doesn't undefine __cplusplus, so the included headers define a template
> > (because __cplusplus is defined), but then that code is in an 'extern
> > "C"' block, and explosion.
> >
> The commit does sound a bit off, admittedly I don't fully grok what
> you're trying to say.
> 
> The extern "C" { } construct annotates anything within as if it were a
> normal C code. Thus functions will have the C linkage (otherwise
> they'll have the C++ mangling) and structs are using the C rules.
> So if you have a C++ header further down the chain it will be
> considered as C and hell will break loose.
> 
> This patch is correct, just sent a fix for glapitable.h, while glapi.h
> was handled with d38839a498b38b745f5bdf6b96fbafa2792670be.
> 
> As-is "struct _glapi_table" will be interpret as C++ one, which may
> not have the same guarantees about sizeof and pointer arithmetic that
> we're using in the test.
> 
> Hope the above provides some inspiration towards a better commit message.
> 
> Thanks
> Emil

I think that I put the wrong commit message on this (or another change has made
this commit message non-sense). Essentially what I was seeing was one of the
headers included had:

#ifdef __cplusplus
template ...
#endif

Which obviously doesn't work inside an extern "C".


signature.asc
Description: signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/8] glapi/check_table: Remove 'extern "C"' block

[Emil Velikov]

On 20 November 2017 at 23:12, Dylan Baker  wrote:
> This doesn't actually accomplish what it's meant to do, as extern C
> doesn't undefine __cplusplus, so the included headers define a template
> (because __cplusplus is defined), but then that code is in an 'extern
> "C"' block, and explosion.
>
The commit does sound a bit off, admittedly I don't fully grok what
you're trying to say.

The extern "C" { } construct annotates anything within as if it were a
normal C code. Thus functions will have the C linkage (otherwise
they'll have the C++ mangling) and structs are using the C rules.
So if you have a C++ header further down the chain it will be
considered as C and hell will break loose.

This patch is correct, just sent a fix for glapitable.h, while glapi.h
was handled with d38839a498b38b745f5bdf6b96fbafa2792670be.

As-is "struct _glapi_table" will be interpret as C++ one, which may
not have the same guarantees about sizeof and pointer arithmetic that
we're using in the test.

Hope the above provides some inspiration towards a better commit message.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 3/8] glapi/check_table: Remove 'extern "C"' block

[Dylan Baker]

This doesn't actually accomplish what it's meant to do, as extern C
doesn't undefine __cplusplus, so the included headers define a template
(because __cplusplus is defined), but then that code is in an 'extern
"C"' block, and explosion.

Signed-off-by: Dylan Baker 
---
 src/mapi/glapi/tests/check_table.cpp | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/mapi/glapi/tests/check_table.cpp 
b/src/mapi/glapi/tests/check_table.cpp
index 62b3a43d22f..7cded8d352f 100644
--- a/src/mapi/glapi/tests/check_table.cpp
+++ b/src/mapi/glapi/tests/check_table.cpp
@@ -24,10 +24,8 @@
 #include 
 #include "main/glheader.h"
 
-extern "C" {
 #include "glapi/glapi.h"
 #include "glapi/glapitable.h"
-}
 
 struct name_offset {
const char *name;
-- 
2.15.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev