Add warning for c++ member variable shadowing

[Richard Smith via cfe-commits]

+def warn_shadow_member_variable : Warning<
+  "shadowed variable '%0' in type '%1' inheriting from type '%2'">,

The phrasing of this is incorrect: the things you're warning about are not
variables, they're non-static data members. Perhaps something like:

  "non-static data member '%0' of '%1' shadows member inherited from type
'%2'"

+   InGroup<Shadow>;

Would it make sense to put this in a subgroup of -Wshadow so that it can be
controlled separately?

+  /// Check if there is a member variable shadowing

Please end comments in a period.

+  void CheckShadowInheritedVariables(const SourceLocation &Loc,

Likewise, 'Variables' is wrong. We would typically use the C term 'Fields'
for these cases within Clang sources.

+  for (const auto &Base : DC->bases()) {
+    if (const auto *TSI = Base.getTypeSourceInfo())
+      if (const auto *BaseClass = TSI->getType()->getAsCXXRecordDecl()) {
+        for (const auto *Field : BaseClass->fields())
+          if (Field->getName() == FieldName)
+            Diag(Loc, diag::warn_shadow_member_variable)
+              << FieldName << RD->getName() << BaseClass->getName();
+        // Search parent's parents
+        CheckShadowInheritedVariables(Loc, FieldName, RD, BaseClass);
+      }
+  }

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.

On 24 January 2017 at 20:52, James Sun <jamessun at fb.com> wrote:

> Thanks for the comments. The new version is attached.
>
> Wrt two of your questions:
>
>
>
> (1)  “The description that you have on 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.”
>
> I’ve read through the doxygen wiki; hopefully it’s fixed; let me know if
> it’s still wrong
>
>
>
> (2) “Why are you checking that the DeclContext has a definition rather
> than the record itself?”
>
> 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.
>
>
>
> Thanks
>
>
>
> James
>
>
>
>
>
> *From: *Saleem Abdulrasool <compnerd at compnerd.org>
> *Date: *Tuesday, January 24, 2017 at 7:10 PM
> *To: *James Sun <jamessun at fb.com>
> *Cc: *"cfe-commits at lists.llvm.org" <cfe-commits at lists.llvm.org>, Aaron
> Ballman <aaron at aaronballman.com>, Richard Smith <richard at metafoo.co.uk>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> Some more stylistic comments:
>
>
>
> The description that you have on 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.
>
>
>
> The ignore warning comments are restating what is in the code, please
> remove them.
>
>
>
> Could you make the header and the source file match the name?
>
>
>
> Why are you checking that the DeclContext has a definition rather than the
> record itself?
>
>
>
> Space after the <<.
>
>
>
> 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:
>
>
>
>     if (const auto *RD = cast<CXXRecordDecl>(CurContext))
>
>       CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
>
>
>
>
>
>
>
> On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
> Coding style change
>
>
>
> *From: *James Sun <jamessun at fb.com>
> *Date: *Tuesday, January 24, 2017 at 2:36 PM
> *To: *"cfe-commits at lists.llvm.org" <cfe-commits at lists.llvm.org>
> *Subject: *Add warning for c++ member variable shadowing
>
>
>
> Dear members
>
>
>
> 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.
>
>
>
> class a {
>
>   int foo;
>
> }
>
>
>
> class b : a {
>
>   int foo; // Generate a warning
>
> }
>
>
>
> This patch
>
> (1) adds a member variable shadowing checking, and
>
> (2) incorporates it to the unit tests.
>
>
>
>
>
> Comments are welcome.
>
>
>
> Thanks
>
>
>
> James
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> <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=>
>
>
>
>
>
> --
>
> Saleem Abdulrasool
> compnerd (at) compnerd (dot) org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170127/4ce1c7de/attachment-0001.html>