Patchwork restore bootstrap with a C++ compiler

login
register
mail settings
Submitter Andrew Pinski
Date April 28, 2011, 2:05 a.m.
Message ID <BANLkTim1B1_FqkSj6Xy3h6dT5nJw0W39Tg@mail.gmail.com>
Download mbox | patch
Permalink /patch/93164/
State New
Headers show

Comments

Andrew Pinski - April 28, 2011, 2:05 a.m.
On Wed, Apr 27, 2011 at 6:48 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
> A local `extern' declaration does not give the entity an external linkage
> -- irrespective of the linkage of the function enclosing the declaration.
> It just makes the name locally available for name lookup
> purpose.   A variable declared const has
>  -- external linkage by default in C
>  -- internal linkage by default in C++
>
> So before the patch: the variables had external linkage in C, but
> internal linkage in C++.  That meant that a link will fail in C++, but
> succeeds in C.  All my patch did was to give the external linkage
> explicitly (both in C and in C++.)

Then I think a better fix is to do:
So nobody is tempted to use those arrays directly from the code but
rather only use the static inline functions.

Thanks,
Andrew Pinski
Gabriel Dos Reis - April 28, 2011, 2:24 a.m.
Andrew Pinski <pinskia@gmail.com> writes:

| On Wed, Apr 27, 2011 at 6:48 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
| > A local `extern' declaration does not give the entity an external linkage
| > -- irrespective of the linkage of the function enclosing the declaration.
| > It just makes the name locally available for name lookup
| > purpose.   A variable declared const has
| >  -- external linkage by default in C
| >  -- internal linkage by default in C++
| >
| > So before the patch: the variables had external linkage in C, but
| > internal linkage in C++.  That meant that a link will fail in C++, but
| > succeeds in C.  All my patch did was to give the external linkage
| > explicitly (both in C and in C++.)
| 
| Then I think a better fix is to do:
| Index: internal-fn.c
| ===================================================================
| --- internal-fn.c	(revision 172940)
| +++ internal-fn.c	(working copy)
| @@ -27,6 +27,7 @@ along with GCC; see the file COPYING3.
|  #include "gimple.h"
| 
|  /* The names of each internal function, indexed by function number.  */
| +extern const char *const internal_fn_name_array[];
|  const char *const internal_fn_name_array[] = {
|  #define DEF_INTERNAL_FN(CODE, FLAGS) #CODE,
|  #include "internal-fn.def"
| @@ -35,6 +36,7 @@ const char *const internal_fn_name_array
|  };
| 
|  /* The ECF_* flags of each internal function, indexed by function number.  */
| +extern const int internal_fn_flags_array[];
|  const int internal_fn_flags_array[] = {
|  #define DEF_INTERNAL_FN(CODE, FLAGS) FLAGS,
|  #include "internal-fn.def"
| --- CUT ---
| So nobody is tempted to use those arrays directly from the code but
| rather only use the static inline functions.

Well, anybody who can put those extern declarations anywhere and use
them directly.  The real issue is that the array variables have external
linkage.

So, I think the patch is largely stylistic but equivalent; I'll defer to
you which one should "prevail".

-- Gaby
Richard Sandiford - April 28, 2011, 7:41 a.m.
Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
> Andrew Pinski <pinskia@gmail.com> writes:
>
> | On Wed, Apr 27, 2011 at 6:48 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
> | > A local `extern' declaration does not give the entity an external linkage
> | > -- irrespective of the linkage of the function enclosing the declaration.
> | > It just makes the name locally available for name lookup
> | > purpose.   A variable declared const has
> | >  -- external linkage by default in C
> | >  -- internal linkage by default in C++
> | >
> | > So before the patch: the variables had external linkage in C, but
> | > internal linkage in C++.  That meant that a link will fail in C++, but
> | > succeeds in C.  All my patch did was to give the external linkage
> | > explicitly (both in C and in C++.)
> | 
> | Then I think a better fix is to do:
> | Index: internal-fn.c
> | ===================================================================
> | --- internal-fn.c	(revision 172940)
> | +++ internal-fn.c	(working copy)
> | @@ -27,6 +27,7 @@ along with GCC; see the file COPYING3.
> |  #include "gimple.h"
> | 
> |  /* The names of each internal function, indexed by function number.  */
> | +extern const char *const internal_fn_name_array[];
> |  const char *const internal_fn_name_array[] = {
> |  #define DEF_INTERNAL_FN(CODE, FLAGS) #CODE,
> |  #include "internal-fn.def"
> | @@ -35,6 +36,7 @@ const char *const internal_fn_name_array
> |  };
> | 
> |  /* The ECF_* flags of each internal function, indexed by function number.  */
> | +extern const int internal_fn_flags_array[];
> |  const int internal_fn_flags_array[] = {
> |  #define DEF_INTERNAL_FN(CODE, FLAGS) FLAGS,
> |  #include "internal-fn.def"
> | --- CUT ---
> | So nobody is tempted to use those arrays directly from the code but
> | rather only use the static inline functions.
>
> Well, anybody who can put those extern declarations anywhere and use
> them directly.  The real issue is that the array variables have external
> linkage.
>
> So, I think the patch is largely stylistic but equivalent; I'll defer to
> you which one should "prevail".

FWIW, I prefer Andrew's patch, but since yours has been applied,
I suppose there's no point changing it.

Richard
Gabriel Dos Reis - April 28, 2011, 1:43 p.m.
Richard Sandiford <richard.sandiford@linaro.org> writes:

| Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
| > Andrew Pinski <pinskia@gmail.com> writes:
| >
| > | On Wed, Apr 27, 2011 at 6:48 PM, Gabriel Dos Reis <gdr@cs.tamu.edu> wrote:
| > | > A local `extern' declaration does not give the entity an external linkage
| > | > -- irrespective of the linkage of the function enclosing the declaration.
| > | > It just makes the name locally available for name lookup
| > | > purpose.   A variable declared const has
| > | >  -- external linkage by default in C
| > | >  -- internal linkage by default in C++
| > | >
| > | > So before the patch: the variables had external linkage in C, but
| > | > internal linkage in C++.  That meant that a link will fail in C++, but
| > | > succeeds in C.  All my patch did was to give the external linkage
| > | > explicitly (both in C and in C++.)
| > | 
| > | Then I think a better fix is to do:
| > | Index: internal-fn.c
| > | ===================================================================
| > | --- internal-fn.c	(revision 172940)
| > | +++ internal-fn.c	(working copy)
| > | @@ -27,6 +27,7 @@ along with GCC; see the file COPYING3.
| > |  #include "gimple.h"
| > | 
| > |  /* The names of each internal function, indexed by function number.  */
| > | +extern const char *const internal_fn_name_array[];
| > |  const char *const internal_fn_name_array[] = {
| > |  #define DEF_INTERNAL_FN(CODE, FLAGS) #CODE,
| > |  #include "internal-fn.def"
| > | @@ -35,6 +36,7 @@ const char *const internal_fn_name_array
| > |  };
| > | 
| > |  /* The ECF_* flags of each internal function, indexed by function number.  */
| > | +extern const int internal_fn_flags_array[];
| > |  const int internal_fn_flags_array[] = {
| > |  #define DEF_INTERNAL_FN(CODE, FLAGS) FLAGS,
| > |  #include "internal-fn.def"
| > | --- CUT ---
| > | So nobody is tempted to use those arrays directly from the code but
| > | rather only use the static inline functions.
| >
| > Well, anybody who can put those extern declarations anywhere and use
| > them directly.  The real issue is that the array variables have external
| > linkage.
| >
| > So, I think the patch is largely stylistic but equivalent; I'll defer to
| > you which one should "prevail".
| 
| FWIW, I prefer Andrew's patch, but since yours has been applied,
| I suppose there's no point changing it.

when we come to agree on coding style guidelines for GCC in C++, I hope we
recommend against local extern declarations.

-- Gaby
Richard Sandiford - April 28, 2011, 1:46 p.m.
Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
> | FWIW, I prefer Andrew's patch, but since yours has been applied,
> | I suppose there's no point changing it.
>
> when we come to agree on coding style guidelines for GCC in C++, I hope we
> recommend against local extern declarations.

Oh, for pure C++, I definitely agree.  This sort of thing ought to
be marked as private or some such in a C++ context.  But while we're
confining ourselves to a subset of C, I think local externs are a useful
way of hiding implementation details.

Richard
Gabriel Dos Reis - April 28, 2011, 2:25 p.m.
Richard Sandiford <richard.sandiford@linaro.org> writes:

| Gabriel Dos Reis <gdr@cs.tamu.edu> writes:
| > | FWIW, I prefer Andrew's patch, but since yours has been applied,
| > | I suppose there's no point changing it.
| >
| > when we come to agree on coding style guidelines for GCC in C++, I hope we
| > recommend against local extern declarations.
| 
| Oh, for pure C++, I definitely agree.  This sort of thing ought to
| be marked as private or some such in a C++ context.  But while we're
| confining ourselves to a subset of C, I think local externs are a useful
| way of hiding implementation details.


well, please apply Andrew's and revert mine then.

-- Gaby

Patch

Index: internal-fn.c
===================================================================
--- internal-fn.c	(revision 172940)
+++ internal-fn.c	(working copy)
@@ -27,6 +27,7 @@  along with GCC; see the file COPYING3.
 #include "gimple.h"

 /* The names of each internal function, indexed by function number.  */
+extern const char *const internal_fn_name_array[];
 const char *const internal_fn_name_array[] = {
 #define DEF_INTERNAL_FN(CODE, FLAGS) #CODE,
 #include "internal-fn.def"
@@ -35,6 +36,7 @@  const char *const internal_fn_name_array
 };

 /* The ECF_* flags of each internal function, indexed by function number.  */
+extern const int internal_fn_flags_array[];
 const int internal_fn_flags_array[] = {
 #define DEF_INTERNAL_FN(CODE, FLAGS) FLAGS,
 #include "internal-fn.def"
--- CUT ---