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<mailto: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<mailto:james...@fb.com>> wrote: Thanks Richard! Good catch! The updated version is attached. --James From: <meta...@gmail.com<mailto:meta...@gmail.com>> on behalf of Richard Smith <rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>> Date: Thursday, February 2, 2017 at 11:59 AM To: James Sun <james...@fb.com<mailto:james...@fb.com>> Cc: Saleem Abdulrasool <compn...@compnerd.org<mailto:compn...@compnerd.org>>, "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman <aa...@aaronballman.com<mailto: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<mailto:james...@fb.com>> wrote: Hi Richard Thanks for the feedback! Hopefully addressed! Thanks James From: <meta...@gmail.com<mailto:meta...@gmail.com>> on behalf of Richard Smith <rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>> Date: Wednesday, February 1, 2017 at 3:50 PM To: James Sun <james...@fb.com<mailto:james...@fb.com>> Cc: Saleem Abdulrasool <compn...@compnerd.org<mailto:compn...@compnerd.org>>, "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman <aa...@aaronballman.com<mailto: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<mailto:james...@fb.com>> wrote: Fixed! From: Saleem Abdulrasool <compn...@compnerd.org<mailto:compn...@compnerd.org>> Date: Tuesday, January 31, 2017 at 6:53 PM To: James Sun <james...@fb.com<mailto:james...@fb.com>> Cc: Richard Smith <rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>>, "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman <aa...@aaronballman.com<mailto: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<mailto: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<mailto:compn...@compnerd.org>> Date: Monday, January 30, 2017 at 6:50 PM To: James Sun <james...@fb.com<mailto:james...@fb.com>> Cc: Richard Smith <rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>>, "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman <aa...@aaronballman.com<mailto: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<mailto: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<mailto:meta...@gmail.com>> on behalf of Richard Smith <rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>> Date: Friday, January 27, 2017 at 3:03 PM To: James Sun <james...@fb.com<mailto:james...@fb.com>> Cc: Saleem Abdulrasool <compn...@compnerd.org<mailto:compn...@compnerd.org>>, "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman <aa...@aaronballman.com<mailto: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<mailto: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<mailto:compn...@compnerd.org>> Date: Tuesday, January 24, 2017 at 7:10 PM To: James Sun <james...@fb.com<mailto:james...@fb.com>> Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman <aa...@aaronballman.com<mailto:aa...@aaronballman.com>>, Richard Smith <rich...@metafoo.co.uk<mailto: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<mailto:cfe-commits@lists.llvm.org>> wrote: Coding style change From: James Sun <james...@fb.com<mailto:james...@fb.com>> Date: Tuesday, January 24, 2017 at 2:36 PM To: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>" <cfe-commits@lists.llvm.org<mailto: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<mailto: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
