[PATCH] Better diagnostic for re-initialization of static data members

[Richard Smith]

I like this change, but I wonder if we can come up with a better wording
for the diagnostic than "re-initialization". Strawman:

"static data member %0 already has an initializer"


On Wed, Nov 20, 2013 at 2:44 PM, Hans Wennborg <hans at chromium.org> wrote:

> Hi rsmith,
>
> +richard, because he put in the fixme in test/CXX/drs/dr0xx.cpp:925.
>
> For the following code:
>   struct S {
>     static const int x = 42;
>   };
>   const int S::x = 42;
>
> This patch changes the diagnostic from:
>
>   a.cc:4:14: error: redefinition of 'x'
>   const int S::x = 42;
>                ^
>   a.cc:2:20: note: previous definition is here
>     static const int x = 42;
>                      ^
> to:
>
>   a.cc:4:18: error: re-initialization of static data member 'x'
>   const int S::x = 42;
>                    ^
>   a.cc:2:24: note: previous initialization is here
>     static const int x = 42;
>                          ^
>
> http://llvm-reviews.chandlerc.com/D2235
>
> Files:
>   include/clang/Basic/DiagnosticSemaKinds.td
>   lib/Sema/SemaDecl.cpp
>   test/CXX/class/class.static/class.static.data/p4.cpp
>   test/CXX/drs/dr0xx.cpp
>   test/SemaCXX/cxx1y-variable-templates_in_class.cpp
>
> Index: include/clang/Basic/DiagnosticSemaKinds.td
> ===================================================================
> --- include/clang/Basic/DiagnosticSemaKinds.td
> +++ include/clang/Basic/DiagnosticSemaKinds.td
> @@ -3525,6 +3525,8 @@
>  def warn_missing_variable_declarations : Warning<
>    "no previous extern declaration for non-static variable %0">,
>    InGroup<DiagGroup<"missing-variable-declarations">>, DefaultIgnore;
> +def err_static_data_member_reinitialization :
> +  Error<"re-initialization of static data member %0">;
>  def err_redefinition : Error<"redefinition of %0">;
>  def err_alias_after_tentative :
>    Error<"alias definition of %0 after tentative definition">;
> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp
> +++ lib/Sema/SemaDecl.cpp
> @@ -8184,9 +8184,8 @@
>      // data members we also need to check whether there was an in-class
>      // declaration with an initializer.
>      if (VDecl->isStaticDataMember() &&
> VDecl->getAnyInitializer(PrevInit)) {
> -      Diag(VDecl->getLocation(), diag::err_redefinition)
> -        << VDecl->getDeclName();
> -      Diag(PrevInit->getLocation(), diag::note_previous_definition);
> +      Diag(Init->getExprLoc(),
> diag::err_static_data_member_reinitialization) << VDecl->getDeclName();
> +      Diag(PrevInit->getInit()->getExprLoc(),
> diag::note_previous_initializer) << 0;
>        return;
>      }
>
> Index: test/CXX/class/class.static/class.static.data/p4.cpp
> ===================================================================
> --- test/CXX/class/class.static/class.static.data/p4.cpp
> +++ test/CXX/class/class.static/class.static.data/p4.cpp
> @@ -10,15 +10,14 @@
>  int const OutOfClassInitializerOnly::i = 0;
>
>  struct InClassInitializerAndOutOfClassCopyInitializer {
> -  static const int i = 0; // expected-note{{previous definition is here}}
> +  static const int i = 0; // expected-note{{previous initialization is
> here}}
>  };
> -int const InClassInitializerAndOutOfClassCopyInitializer::i = 0; //
> expected-error{{redefinition of 'i'}}
> +int const InClassInitializerAndOutOfClassCopyInitializer::i = 0; //
> expected-error{{re-initialization of static data member 'i'}}
>
>  struct InClassInitializerAndOutOfClassDirectInitializer {
> -  static const int i = 0; // expected-note{{previous definition is here}}
> +  static const int i = 0; // expected-note{{previous initialization is
> here}}
>  };
> -int const InClassInitializerAndOutOfClassDirectInitializer::i(0); //
> expected-error{{redefinition of 'i'}}
> -
> +int const InClassInitializerAndOutOfClassDirectInitializer::i(0); //
> expected-error{{re-initialization of static data member 'i'}}
>
>
>  int main() { }
> Index: test/CXX/drs/dr0xx.cpp
> ===================================================================
> --- test/CXX/drs/dr0xx.cpp
> +++ test/CXX/drs/dr0xx.cpp
> @@ -919,11 +919,10 @@
>
>  namespace dr88 { // dr88: yes
>    template<typename T> struct S {
> -    static const int a = 1;
> +    static const int a = 1; // expected-note {{previous}}
>      static const int b;
>    };
> -  // FIXME: This diagnostic is pretty bad.
> -  template<> const int S<int>::a = 4; // expected-error {{redefinition}}
> expected-note {{previous}}
> +  template<> const int S<int>::a = 4; // expected-error
> {{re-initialization}}
>    template<> const int S<int>::b = 4;
>  }
>
> Index: test/SemaCXX/cxx1y-variable-templates_in_class.cpp
> ===================================================================
> --- test/SemaCXX/cxx1y-variable-templates_in_class.cpp
> +++ test/SemaCXX/cxx1y-variable-templates_in_class.cpp
> @@ -39,11 +39,11 @@
>    template<typename T> CONST T B1::right<T,int> = T(5);
>
>    class B2 {
> -    template<typename T, typename T0> static CONST T right = T(100);  //
> expected-note {{previous definition is here}}
> -    template<typename T> static CONST T right<T,int> = T(5);          //
> expected-note {{previous definition is here}}
> +    template<typename T, typename T0> static CONST T right = T(100);  //
> expected-note {{previous initialization is here}}
> +    template<typename T> static CONST T right<T,int> = T(5);          //
> expected-note {{previous initialization is here}}
>    };
> -  template<typename T, typename T0> CONST T B2::right = T(100);   //
> expected-error {{redefinition of 'right'}}
> -  template<typename T> CONST T B2::right<T,int> = T(5);           //
> expected-error {{redefinition of 'right'}}
> +  template<typename T, typename T0> CONST T B2::right = T(100);   //
> expected-error {{re-initialization of static data member 'right'}}
> +  template<typename T> CONST T B2::right<T,int> = T(5);           //
> expected-error {{re-initialization of static data member 'right'}}
>
>    class B3 {
>      template<typename T, typename T0> static CONST T right = T(100);
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131120/b9ad3f71/attachment.html>