[PATCH] attempt for bug 6904 - clang allows redefinition of static const member

Hello All :slight_smile:

This is my first attempt at patching Clang.

This patch is supposed to address the following bug (described as
'easy to fix' by CL in his comment):
http://llvm.org/bugs/show_bug.cgi?id=6904#c2

Essentially clang allows the following redefinition, and it shouldn't :
struct S { static const int i = 0; }; const int S::i = 5;

I am a simple hobbyist, and would appreciate it if someone could
critique or guide me through some of this.

My approach was the following:

In file 'SemaDecl.cpp' in the function
void Sema::AddInitializerToDecl(DeclPtrTy dcl, ExprArg init, bool DirectInit)

.. as soon as we get a valid VarDecl*, before we add an initializer to
it, i simply iterate through all its prior declarations and if any of
them has an initializer, i invalidate the current VDecl and return.

Here is the code:
VarDecl *VDecl = dyn_cast<VarDecl>(RealDecl);
if (!VDecl) {
....
}
//----- My Simple Addition

  VarDecl *First = VDecl->getFirstDeclaration();
  for (VarDecl::redecl_iterator I = First->redecls_begin(), E =
First->redecls_end();
    I != E; ++I) {
    if ((*I)->hasInit())
    {
      Diag(VDecl->getLocation(), diag::err_redefinition) <<
VDecl->getDeclName();
      Diag((*I)->getLocation(), diag::note_previous_definition);
      VDecl->setInvalidDecl();
      return;
    }
  }

//-----------------------------------------------------

Here are some of my questions:
1) Is this the best place to insert this code?
2) Can I avoid having to use a loop and just query a previous
declaration? Or is it more robust to loop through?
i.e. VDecl->getPreviousDeclaration()->hasInit()?

Any thoughts or feedback will be appreciated.

Thanks!
Faisal Vali.

Essentially clang allows the following redefinition, and it shouldn't :
struct S { static const int i = 0; }; const int S::i = 5;

In file 'SemaDecl.cpp' in the function
void Sema::AddInitializerToDecl(DeclPtrTy dcl, ExprArg init, bool DirectInit)

.. as soon as we get a valid VarDecl*, before we add an initializer to
it, i simply iterate through all its prior declarations and if any of
them has an initializer, i invalidate the current VDecl and return.

Apparently the above check does not catch 'const int S::i(0)' so it
needs to be added to the following function too

In file SemDeclCXX.cpp void Sema::AddCXXDirectInitializerToDecl(...)

This makes me wonder if there is a better place to add this code so as
to avoid repeating ourselves?
Also once list initialization comes into the picture, will we need
another check for that case?

Thanks!
Faisal Vali
Radiation Oncology
Loyola

Thanks for the tip - i have attached a patch file (using tortoiseSVN).

regards,
Faisal Vali

int-static-var-redef-bug.patch (2.37 KB)

Index: SemaDecl.cpp

Aah - thanks for the tip - yes that would be clearer. I was avoiding
getPreviousDeclaration for fear of missing some corner case that might
lead to multiple previous declarations of such variables - but
getAnyInitializer should be robust enough.

Thanks - I will post a patch in the evening to this group - unless you
want me to post it to cfe-commit.

regards,

Please post to cfe-commits, thanks!

  - Doug

Hi Faisal,

Just FYI, there's another accepting-invalid-code bug regarding static data members, that should also be easy to fix, if you are interested:
http://llvm.org/PR7970

-Argiris