<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div class="gmail_quote">+def warn_shadow_member_variable : Warning<</div><div class="gmail_quote">+ "shadowed variable '%0' in type '%1' inheriting from type '%2'">,</div><div class="gmail_quote"><br></div><div class="gmail_quote">The phrasing of this is incorrect: the things you're warning about are not variables, they're non-static data members. Perhaps something like:</div><div class="gmail_quote"><br></div><div class="gmail_quote"> "non-static data member '%0' of '%1' shadows member inherited from type '%2'"</div><div class="gmail_quote"><br></div><div class="gmail_quote">+ InGroup<Shadow>;</div><div class="gmail_quote"><br></div><div class="gmail_quote">Would it make sense to put this in a subgroup of -Wshadow so that it can be controlled separately?</div><div class="gmail_quote"><br></div><div class="gmail_quote"><div class="gmail_quote">+ /// Check if there is a member variable shadowing</div><div class="gmail_quote"><br></div><div class="gmail_quote">Please end comments in a period.</div><div class="gmail_quote"><br></div><div class="gmail_quote">+ void CheckShadowInheritedVariables(const SourceLocation &Loc,</div><div class="gmail_quote"><br></div><div class="gmail_quote">Likewise, 'Variables' is wrong. We would typically use the C term 'Fields' for these cases within Clang sources.</div><div class="gmail_quote"><br></div><div class="gmail_quote"><div class="gmail_quote">+ for (const auto &Base : DC->bases()) {<br></div></div><div class="gmail_quote"><div class="gmail_quote">+ if (const auto *TSI = Base.getTypeSourceInfo())</div><div class="gmail_quote">+ if (const auto *BaseClass = TSI->getType()->getAsCXXRecordDecl()) {</div><div class="gmail_quote">+ for (const auto *Field : BaseClass->fields())</div><div class="gmail_quote">+ if (Field->getName() == FieldName)</div><div class="gmail_quote">+ Diag(Loc, diag::warn_shadow_member_variable)</div><div class="gmail_quote">+ << FieldName << RD->getName() << BaseClass->getName();</div><div class="gmail_quote">+ // Search parent's parents</div><div class="gmail_quote">+ CheckShadowInheritedVariables(Loc, FieldName, RD, BaseClass);</div><div class="gmail_quote">+ }</div><div class="gmail_quote">+ }</div><div><br></div><div>Maybe we should avoid diagnosing shadowing of members that are inaccessible from the derived class? What about if the field name is ambiguous? Also, we shouldn't recurse if lookup finds something with the given name in this class, and ideally we would only visit each class once, even if it appears multiple times in a multiple-inheritance scenario. CXXRecordDecl::lookupInBases can handle most of these cases for you automatically, and will also let you build a set of paths to problematic base classes in case you want to report those.</div></div></div></div><div class="gmail_quote"><br></div><div class="gmail_quote">On 24 January 2017 at 20:52, James Sun <span dir="ltr"><<a href="mailto:jamessun@fb.com" target="_blank">jamessun@fb.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div bgcolor="white" lang="EN-US">
<div class="gmail-m_-2479753666174409069WordSection1">
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri">Thanks for the comments. The new version is attached.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri">Wrt two of your questions:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri">(1) “The description that you have on <wbr>CheckShadowInheritedVariables isn't really the type of comments that we have in doxygen form. Im not sure if its in line with the rest of the code.”<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri">I’ve read through the doxygen wiki; hopefully it’s fixed; let me know if it’s still wrong<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri">(2) “Why are you checking that the DeclContext has a definition rather than the record itself?”<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri">There are cases like “struct A; struct B : A {};”, where A does not have a definition. The compiler will hit an assertion failure if we call A.bases() directly.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri">Thanks<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri">James<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:calibri"><u></u> <u></u></span></p>
<div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(181,196,223);padding:3pt 0in 0in">
<p class="MsoNormal"><b><span style="font-family:calibri;color:black">From: </span>
</b><span style="font-family:calibri;color:black">Saleem Abdulrasool <<a href="mailto:compnerd@compnerd.org" target="_blank">compnerd@compnerd.org</a>><br>
<b>Date: </b>Tuesday, January 24, 2017 at 7:10 PM<br>
<b>To: </b>James Sun <<a href="mailto:jamessun@fb.com" target="_blank">jamessun@fb.com</a>><br>
<b>Cc: </b>"<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>" <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>>, Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>>, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
<b>Subject: </b>Re: Add warning for c++ member variable shadowing<u></u><u></u></span></p>
</div><div><div class="gmail-h5">
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Some more stylistic comments: <u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">The description that you have on <wbr>CheckShadowInheritedVariables isn't really the type of comments that we have in doxygen form. Im not sure if its in line with the rest of the code.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">The ignore warning comments are restating what is in the code, please remove them.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Could you make the header and the source file match the name?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Why are you checking that the DeclContext has a definition rather than the record itself?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Space after the <<.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Don't use the cast for the check, use isa. Although, since you use the value later, it is probably better to write this as:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"> if (const auto *RD = cast<CXXRecordDecl>(<wbr>CurContext))<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> CheckShadowInheritedVariabless<wbr>(Loc, Name.getAsString(), RD, RD);<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<u></u><u></u></p>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal"><span style="font-size:11pt">Coding style change</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(181,196,223);padding:3pt 0in 0in">
<p class="MsoNormal"><b><span style="color:black">From:
</span></b><span style="color:black">James Sun <<a href="mailto:jamessun@fb.com" target="_blank">jamessun@fb.com</a>><br>
<b>Date: </b>Tuesday, January 24, 2017 at 2:36 PM<br>
<b>To: </b>"<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>" <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>><br>
<b>Subject: </b>Add warning for c++ member variable shadowing</span><u></u><u></u></p>
</div>
<div>
<div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<p class="MsoNormal"><span style="font-size:11pt">Dear members</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">Here is a patch (attached) to create warnings where a member variable shadows the one in one of its inheriting classes. For cases where we really
don't want to shadow member variables, e.g. </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">class a {</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> int foo;
</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">}</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">class b : a {
</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> int foo; // Generate a warning</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">}</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">This patch
</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">(1) adds a member variable shadowing checking, and
</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">(2) incorporates it to the unit tests.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">Comments are welcome.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">Thanks</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt">James</span><u></u><u></u></p>
</div>
</div>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12pt"><br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_cfe-2Dcommits&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=ikRH8URaurZA7JMys57d3w&m=lheFEjRie_ahss0mWHaJIa1eNMlFv2DMH5ZWHGQvo8U&s=750RLygVMQIDJB7IKBhOef4zIDHerGwb7aJZAY2aP9U&e=" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><u></u><u></u></p>
</blockquote>
</div>
<p class="MsoNormal"><br>
<br clear="all">
<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<p class="MsoNormal">-- <u></u><u></u></p>
<div>
<p class="MsoNormal">Saleem Abdulrasool<br>
compnerd (at) compnerd (dot) org<u></u><u></u></p>
</div>
</div>
</div></div></div>
</div>
</blockquote></div><br></div></div>