Great, thanks!
On 7 February 2017 at 10:54, James Sun <james...@fb.com> wrote:
> Hi Richard, Saleem
>
>
>
> I cleaned up the patch by removing some unrelated unit tests. Also Saleem
> can help me with the commit.
>
>
>
> Thanks!
>
>
>
> James
>
>
>
> *From: *James Sun <james...@fb.com>
> *Date: *Saturday, February 4, 2017 at 11:35 PM
>
> *To: *Richard Smith <rich...@metafoo.co.uk>
> *Cc: *Saleem Abdulrasool <compn...@compnerd.org>, "
> cfe-commits@lists.llvm.org" <cfe-commits@lists.llvm.org>, Aaron Ballman <
> aa...@aaronballman.com>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> Thanks Richard! Hopefully this is the last patch :D
>
> Could you please help me to commit it maybe?
>
>
>
> Thanks
>
>
>
> James
>
>
>
> *From: *<meta...@gmail.com> on behalf of Richard Smith <
> rich...@metafoo.co.uk>
> *Date: *Saturday, February 4, 2017 at 10:43 PM
> *To: *James Sun <james...@fb.com>
> *Cc: *Saleem Abdulrasool <compn...@compnerd.org>, "
> cfe-commits@lists.llvm.org" <cfe-commits@lists.llvm.org>, Aaron Ballman <
> aa...@aaronballman.com>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> Thanks, just one more thing I noticed (sorry!) and this looks good to go.
>
>
>
> +def warn_shadow_field : Warning<
>
> + "non-static data member '%0' of '%1' shadows member inherited from type
> '%2'">,
>
> + InGroup<ShadowIvar>;
>
>
>
> -Wshadow-ivar doesn't really make sense for this; that's for an
> Objective-C feature. A new -Wshadow-field group might make sense
> (especially since we already have -Wshadow-field-in-constructor). Also,
> the other -Wshadow warnings (other than -Wshadow-ivar) are DefaultIgnore;
> this one should probably be too.
>
>
>
> Do you need someone to commit for you?
>
>
>
> On 4 February 2017 at 22:21, James Sun <james...@fb.com> wrote:
>
> oops
>
>
>
> *From: *James Sun <james...@fb.com>
> *Date: *Saturday, February 4, 2017 at 9:19 PM
>
>
> *To: *Richard Smith <rich...@metafoo.co.uk>
> *Cc: *Saleem Abdulrasool <compn...@compnerd.org>, "
> cfe-commits@lists.llvm.org" <cfe-commits@lists.llvm.org>, Aaron Ballman <
> aa...@aaronballman.com>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> updated
>
>
>
> *From: *James Sun <james...@fb.com>
> *Date: *Saturday, February 4, 2017 at 6:52 PM
> *To: *Richard Smith <rich...@metafoo.co.uk>
> *Cc: *Saleem Abdulrasool <compn...@compnerd.org>, "
> cfe-commits@lists.llvm.org" <cfe-commits@lists.llvm.org>, Aaron Ballman <
> aa...@aaronballman.com>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> Ok I get your point. Suppose there are two paths from class B to base
> class A. One is with access as_none; the other is as_public. Then there is
> a chance that the as_none path is recorded and the as_public one is
> skipped. In this case we should give the warning as well. Should be easy to
> fix with the existing map. Will do.
>
> Sent from my iPhone
>
>
> On Feb 4, 2017, at 6:22 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
>
> Hmm, now we're emitting one warning per path, it looks like we might
> diagnose shadowing the same field more than once (for a repeated
> non-virtual base class). Is that intentional? Maybe we should just skip
> producing the warning if the lookup would be ambiguous, since any use of
> the shadowed field would otherwise be ill-formed.
>
>
>
> On 4 February 2017 at 17:48, James Sun <james...@fb.com> wrote:
>
> Thanks Richard! Good catch! The updated version is attached. --James
>
>
>
> *From: *<meta...@gmail.com> on behalf of Richard Smith <
> rich...@metafoo.co.uk>
> *Date: *Thursday, February 2, 2017 at 11:59 AM
>
> *To: *James Sun <james...@fb.com>
> *Cc: *Saleem Abdulrasool <compn...@compnerd.org>, "
> cfe-commits@lists.llvm.org" <cfe-commits@lists.llvm.org>, Aaron Ballman <
> aa...@aaronballman.com>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> Thanks, James! I think I have only one more substantive comment:
>
>
>
> + (Field->getAccess() == AS_public || Field->getAccess() ==
> AS_protected)) {
>
>
>
> Have you considered also taking into account the access of the inheritance
> path? Eg, a public member of a private base class of a public base class is
> typically inaccessible, even though it was declared public:
>
>
>
> struct A { int n; };
>
> struct B : private A {};
>
> struct C : B { int n; }; // A::n is not accessible here, should we
> suppress the warning?
>
>
>
> You can use CXXRecordDecl::MergeAccess to combine the access of the path
> with the access of the field and compute the effective access of the field
> in the derived class (and you should test to see if the resulting access is
> AS_None to tell if the field is inaccessible; fields with effective access
> of AS_Private -- such as public members of a private direct base class --
> are accessible from the derived class). You'll need to set RecordPaths to
> true in the CXXBasePaths object in order for lookupInBases to compute the
> path access.
>
>
>
> Oh, and you may as well use a range-based for loop here:
>
>
>
> + auto Result = Base->lookup(FieldName);
>
> + for (auto I = Result.begin(); I != Result.end(); ++I) {
>
>
>
>
>
> On 2 February 2017 at 00:19, James Sun <james...@fb.com> wrote:
>
> Hi Richard
>
>
>
> Thanks for the feedback! Hopefully addressed!
>
>
>
> Thanks
>
>
>
> James
>
>
>
>
>
>
>
> *From: *<meta...@gmail.com> on behalf of Richard Smith <
> rich...@metafoo.co.uk>
> *Date: *Wednesday, February 1, 2017 at 3:50 PM
> *To: *James Sun <james...@fb.com>
>
>
> *Cc: *Saleem Abdulrasool <compn...@compnerd.org>, "
> cfe-commits@lists.llvm.org" <cfe-commits@lists.llvm.org>, Aaron Ballman <
> aa...@aaronballman.com>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> + std::set<StringRef> bases;
>
> + const auto baseName = Specifier->getType()->
> getAsCXXRecordDecl()->getName();
>
>
>
> Please capitalize local variable names. Also, please don't use the record
> name as a key in your set; that's not guaranteed to be unique. Instead, you
> could either use a set of canonical types or of canonical CXXRecordDecl*s.
>
>
>
> + for (const auto *Field : Specifier->getType()->
> getAsCXXRecordDecl()->fields()) {
>
> + if ((Field->getAccess() == AS_public || Field->getAccess() ==
> AS_protected) &&
>
> + Field->getName() == FieldName) {
>
>
>
> Use Specifier->getType()->getAsCXXRecordDecl()->lookup(Field->getName())
> here to look up the field by name, rather than walking all the fields of
> all base classes and checking if each of them has the right name. You
> should also check for IndirectFieldDecls, for this case:
>
>
>
> struct A {
>
> union { int x; float f; };
>
> };
>
> struct B : A {
>
> int x;
>
> };
>
>
>
> + bases.emplace(baseName);
>
>
>
> It's more efficient to use insert rather than emplace when inserting an
> element into a set.
>
>
>
> + Diag(Loc, diag::warn_shadow_field)
>
> + << FieldName << RD->getName() << baseName;
>
>
>
> It'd be nice to add a note here pointing at the base class member that was
> shadowed.
>
>
>
>
>
>
>
> On 31 January 2017 at 19:20, James Sun <james...@fb.com> wrote:
>
> Fixed!
>
>
>
> *From: *Saleem Abdulrasool <compn...@compnerd.org>
> *Date: *Tuesday, January 31, 2017 at 6:53 PM
>
>
> *To: *James Sun <james...@fb.com>
> *Cc: *Richard Smith <rich...@metafoo.co.uk>, "cfe-commits@lists.llvm.org"
> <cfe-commits@lists.llvm.org>, Aaron Ballman <aa...@aaronballman.com>
> *Subject: *Re: Add warning for c++ member variable shadowing
>
>
>
> 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 <james...@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 <compn...@compnerd.org>
> *Date: *Monday, January 30, 2017 at 6:50 PM
> *To: *James Sun <james...@fb.com>
> *Cc: *Richard Smith <rich...@metafoo.co.uk>, "cfe-commits@lists.llvm.org"
> <cfe-commits@lists.llvm.org>, Aaron Ballman <aa...@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 <james...@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: *<meta...@gmail.com> on behalf of Richard Smith <
> rich...@metafoo.co.uk>
> *Date: *Friday, January 27, 2017 at 3:03 PM
> *To: *James Sun <james...@fb.com>
> *Cc: *Saleem Abdulrasool <compn...@compnerd.org>, "
> cfe-commits@lists.llvm.org" <cfe-commits@lists.llvm.org>, Aaron Ballman <
> aa...@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 <james...@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 <compn...@compnerd.org>
> *Date: *Tuesday, January 24, 2017 at 7:10 PM
> *To: *James Sun <james...@fb.com>
> *Cc: *"cfe-commits@lists.llvm.org" <cfe-commits@lists.llvm.org>, Aaron
> Ballman <aa...@aaronballman.com>, Richard Smith <rich...@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@lists.llvm.org> wrote:
>
> Coding style change
>
>
>
> *From: *James Sun <james...@fb.com>
> *Date: *Tuesday, January 24, 2017 at 2:36 PM
> *To: *"cfe-commits@lists.llvm.org" <cfe-commits@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@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
>
>
>
>
>
>
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
