[cfe-commits] [PATCH] for bug 7970 and another member declaration bug found along the way

[Douglas Gregor]

On Aug 27, 2010, at 5:32 PM, Faisal Vali wrote:

> This is an updated patch which spits out more consistent sounding
> error messages.
> In Sema::MergeVarDecl, instead of doing (pseudo code)
> if (LR.isSingleResult())
>  PDecl = LR.getFoundDecl()
> I used:
> if (!LR.empty())
>  PDecl = LR.getRepresentativeDecl()
> 
> Thanks!

Your patch looks pretty good. I've simplified the change in Sema::MergeVarDecls quite a bit by checking that we're not redeclaring a static data member *after* we've checked that we aren't performing a redeclaration of entities of a different kind. A few other minor comments:

Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp	(revision 112294)
+++ lib/Sema/SemaDecl.cpp	(working copy)
@@ -1456,7 +1456,28 @@
   // If the new decl is already invalid, don't do any other checking.
   if (New->isInvalidDecl())
     return;
+  // If 'New' is an in-class static data member declaration then 'PrevDecl' 
+  // can only be an *in-class* member declaration (one can not declare an out of class 
+  // name before an inclass name, right?!). 
+  // In such a case 'New' has to be ill-formed since we can only re-declare a name if 
+  // it designates an overload of a member function and we know 'New' is NOT a function - 
+  // so any prior member declaration of the same name has to be an error.

We usually try to keep comments a bit shorter, and avoid questions in them, unless it's something really complicated. I went with the far shorter:

  // C++ [class.mem]p1:
  //   A member shall not be declared twice in the member-specification [...]
  // 
  // Here, we need only consider static data members.

+  if (!Previous.empty()) {
+    NamedDecl* PrevDecl = Previous.getRepresentativeDecl();
 
+    if (New->isStaticDataMember() && !New->isOutOfLine() && PrevDecl->isCXXClassMember())  {   

Please keep lines <= 80 columns.

  int v;  //expected-note{{previous declaration is here}} expected-note{{previous declaration is here}} expected-note{{previous declaration is here}}

Note that you can use:

  // expected-note 3{{previous declaration is here}

to check that the "previous declaration is here" diagnostic occurs three times.

Thanks for the great test case, too!

Revised patch committed as r112476.

	- Doug

> 
> 
> 
> 
> On Fri, Aug 27, 2010 at 6:47 PM, Faisal Vali <faisalv at yahoo.com> wrote:
>> Hi,
>> 
>> This patch fixes the following bugs:
>>  - redeclaration of in class static variables (http://llvm.org/PR7970)
>>  - redeclaration of an in class non-static variable was allowed if
>> *two* or more overloaded functions with that same name had already
>> been declared
>>    i.e. struct S { void f(); int f(int); char f; }; would compile
>> because Sema::LookupSingleResult in Sema::HandleField would return
>> null
>>    if LookupResult was an overloaded result.
>> 
>> I have attached a bunch of tests in p1 (not sure if they are all
>> necessary), and if you review the test file, you'll see that depending
>> on the ordering of member declarations
>>  (i.e data before functions or static before non-static) different
>> error messages are spit out.  Something smells quite brittle about
>> this.  I wonder if it
>> would be worthwhile to try and unify these member declaration checks
>> in one place and then call them consistently from HandleField,
>> HandleDeclarator and MergeVarDecl,
>> or would this complicate more than simplify?
>> 
>> I verified my changes against the tests in SemaCXX and CXX.
>> 
>> I am quite new at this so would appreciate any constructive feedback :)
>> 
>> thanks,
>> Faisal Vali
>> 
> <prevent-static-var-in-class-redeclaration-3.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100830/3f986f4c/attachment.html>