<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Aug 27, 2010, at 5:32 PM, Faisal Vali wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>This is an updated patch which spits out more consistent sounding<br>error messages.<br>In Sema::MergeVarDecl, instead of doing (pseudo code)<br>if (LR.isSingleResult())<br> PDecl = LR.getFoundDecl()<br>I used:<br>if (!LR.empty())<br> PDecl = LR.getRepresentativeDecl()<br><br>Thanks!<br></div></blockquote><div><br></div><div>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:</div><div><br></div><div><div>Index: lib/Sema/SemaDecl.cpp</div><div>===================================================================</div><div>--- lib/Sema/SemaDecl.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 112294)</div><div>+++ lib/Sema/SemaDecl.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -1456,7 +1456,28 @@</div><div> // If the new decl is already invalid, don't do any other checking.</div><div> if (New->isInvalidDecl())</div><div> return;</div><div>+ // If 'New' is an in-class static data member declaration then 'PrevDecl' </div><div>+ // can only be an *in-class* member declaration (one can not declare an out of class </div><div>+ // name before an inclass name, right?!). </div><div>+ // In such a case 'New' has to be ill-formed since we can only re-declare a name if </div><div>+ // it designates an overload of a member function and we know 'New' is NOT a function - </div><div>+ // so any prior member declaration of the same name has to be an error.</div></div><div><br></div><div>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:</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 131, 17); "><span style="color: #000000"> </span>// C++ [class.mem]p1:</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 131, 17); "><span style="color: #000000"> </span>// A member shall not be declared twice in the member-specification [...]</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 131, 17); "><span style="color: #000000"> </span>// </div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; color: rgb(0, 131, 17); "><span style="color: #000000"> </span>// Here, we need only consider static data members.</div><div><br></div><div><div>+ if (!Previous.empty()) {</div><div>+ NamedDecl* PrevDecl = Previous.getRepresentativeDecl();</div><div> </div><div>+ if (New->isStaticDataMember() && !New->isOutOfLine() && PrevDecl->isCXXClassMember()) { </div></div><div><br></div><div>Please keep lines <= 80 columns.</div><div><br></div><div><div> int v; //expected-note{{previous declaration is here}} expected-note{{previous declaration is here}} expected-note{{previous declaration is here}}</div></div><div><br></div><div>Note that you can use:</div><div><br></div><div> // expected-note 3{{previous declaration is here}</div><div><br></div><div>to check that the "previous declaration is here" diagnostic occurs three times.</div><div><br></div><div>Thanks for the great test case, too!</div><div><br></div><div>Revised patch committed as r112476.</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div></div><br><blockquote type="cite"><div><br><br><br><br>On Fri, Aug 27, 2010 at 6:47 PM, Faisal Vali <<a href="mailto:faisalv@yahoo.com">faisalv@yahoo.com</a>> wrote:<br><blockquote type="cite">Hi,<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This patch fixes the following bugs:<br></blockquote><blockquote type="cite"> - redeclaration of in class static variables (<a href="http://llvm.org/PR7970">http://llvm.org/PR7970</a>)<br></blockquote><blockquote type="cite"> - redeclaration of an in class non-static variable was allowed if<br></blockquote><blockquote type="cite">*two* or more overloaded functions with that same name had already<br></blockquote><blockquote type="cite">been declared<br></blockquote><blockquote type="cite"> i.e. struct S { void f(); int f(int); char f; }; would compile<br></blockquote><blockquote type="cite">because Sema::LookupSingleResult in Sema::HandleField would return<br></blockquote><blockquote type="cite">null<br></blockquote><blockquote type="cite"> if LookupResult was an overloaded result.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I have attached a bunch of tests in p1 (not sure if they are all<br></blockquote><blockquote type="cite">necessary), and if you review the test file, you'll see that depending<br></blockquote><blockquote type="cite">on the ordering of member declarations<br></blockquote><blockquote type="cite"> (i.e data before functions or static before non-static) different<br></blockquote><blockquote type="cite">error messages are spit out. Something smells quite brittle about<br></blockquote><blockquote type="cite">this. I wonder if it<br></blockquote><blockquote type="cite">would be worthwhile to try and unify these member declaration checks<br></blockquote><blockquote type="cite">in one place and then call them consistently from HandleField,<br></blockquote><blockquote type="cite">HandleDeclarator and MergeVarDecl,<br></blockquote><blockquote type="cite">or would this complicate more than simplify?<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I verified my changes against the tests in SemaCXX and CXX.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I am quite new at this so would appreciate any constructive feedback :)<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">thanks,<br></blockquote><blockquote type="cite">Faisal Vali<br></blockquote><blockquote type="cite"><br></blockquote><span><prevent-static-var-in-class-redeclaration-3.patch></span>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br></div></blockquote></div><br></body></html>