Message ID | BANLkTim1B1_FqkSj6Xy3h6dT5nJw0W39Tg@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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
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 ---