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 <[email protected]> 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
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
