Add warning for c++ member variable shadowing

Tue Jan 31 18:53:19 PST 2017

Hmm, the braces in the if (bases.find(...)...) are not needed.

Could you also add a test case for virtual inheritance?

On Mon, Jan 30, 2017 at 8:34 PM, James Sun <jamessun at fb.com> wrote:

> Hi Saleem
>
>
>
> Thanks for the quick response. A test case is added. It covers some
> ordinary cases as well as corner cases like multiple paths to the same base.
>
>
>
> Thanks
>
>
>
> James
>
>
>
> *From: *Saleem Abdulrasool <compnerd at compnerd.org>
> *Date: *Monday, January 30, 2017 at 6:50 PM
> *To: *James Sun <jamessun at fb.com>
> *Cc: *Richard Smith <richard at metafoo.co.uk>, "cfe-commits at lists.llvm.org"
> <cfe-commits at lists.llvm.org>, Aaron Ballman <aaron at aaronballman.com>
>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> I think that the patch is starting to look pretty good!
>
>
>
> Can you add some test cases for the particular cases to diagnose in a
> separate test set to ensure that we have proper coverage of the various
> cases rather than relying on the existing test cases?  Something to make
> sure that we get the simple case right as well as the complex cases (e.g.
> we don't print duplicate warnings for multiple paths).
>
>
>
>
>
> On Mon, Jan 30, 2017 at 5:50 PM, James Sun <jamessun at fb.com> wrote:
>
> Hi Richard
>
>
>
> Sorry for the late reply. Thank you for giving the feedback! The updated
> version is attached. Please let me know if there is anything improper.
>
>
>
> Thanks
>
>
>
> James
>
>
>
> *From: *<metafoo at gmail.com> on behalf of Richard Smith <
> richard at metafoo.co.uk>
> *Date: *Friday, January 27, 2017 at 3:03 PM
> *To: *James Sun <jamessun at fb.com>
> *Cc: *Saleem Abdulrasool <compnerd at compnerd.org>, "
> cfe-commits at lists.llvm.org" <cfe-commits at lists.llvm.org>, Aaron Ballman <
> aaron at aaronballman.com>
>
>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> +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
>
>
>
>
>
>
>
> --
>
> Saleem Abdulrasool
> compnerd (at) compnerd (dot) org
>

-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170131/a2080fe9/attachment-0001.html>