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
inheritance-shadow-warning.patch
Description: inheritance-shadow-warning.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Add warning for c++ member variable shadowing
Coding style change
From: James Sun
Date: Tuesday, January 24, 2017 at 2:36 PM
To: "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
inheritance-shadow-warning-v0.2.patch
Description: inheritance-shadow-warning-v0.2.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Add warning for c++ member variable shadowing
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
Date: Tuesday, January 24, 2017 at 7:10 PM
To: James Sun
Cc: "cfe-commits@lists.llvm.org" , Aaron Ballman
, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
inheritance-shadow-warning-v0.3.patch
Description: inheritance-shadow-warning-v0.3.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Add warning for c++ member variable shadowing
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: on behalf of Richard Smith
Date: Friday, January 27, 2017 at 3:03 PM
To: James Sun
Cc: Saleem Abdulrasool , "cfe-commits@lists.llvm.org"
, Aaron Ballman
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;
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
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 mailto:compn...@compnerd.org>>
Date: Tuesday, January 24, 2017 at 7:10 PM
To: James Sun mailto:james...@fb.com>>
Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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 va
Re: Add warning for c++ member variable shadowing
u 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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
inheritance-shadow-warning-v0.5.patch
Description: inheritance-shadow-warning-v0.5.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Add warning for c++ member variable shadowing
:10 PM
To: James Sun mailto:james...@fb.com>>
Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
inheritance-shadow-warning-v0.6.patch
Description: inheritance-shadow-warning-v0.6.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Add warning for c++ member variable shadowing
nst 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
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 mailto:compn...@compnerd.org>>
Date: Tuesday, January 24, 2017 at 7:10 PM
To: James Sun mailto:james...@fb.com>>
Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
inheritance-shadow-warning-v0.7.patch
Description: inheritance-shadow-warning-v0.7.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Add warning for c++ member variable shadowing
t 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
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: mailto:meta...@gmail.com>> on behalf of Richard Smith
mailto:rich...@metafoo.co.uk>>
Date: Friday, January 27, 2017 at 3:03 PM
To: James Sun mailto:james...@fb.com>>
Cc: Saleem Abdulrasool mailto:compn...@compnerd.org>>,
"cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
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;
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
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 mailto:compn...@compnerd.org>>
Date: Tuesday, January 24, 2017 at 7:10 PM
To: James Sun mailto:james...@fb.com>>
Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Codin
Re: Add warning for c++ member variable shadowing
t;
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
Re: Add warning for c++ member variable shadowing
g<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
Re: Add warning for c++ member variable shadowing
f we call A.bases()
directly.
Thanks
James
From: Saleem Abdulrasool mailto:compn...@compnerd.org>>
Date: Tuesday, January 24, 2017 at 7:10 PM
To: James Sun mailto:james...@fb.com>>
Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
Re: Add warning for c++ member variable shadowing
ong
(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 mailto:compn...@compnerd.org>>
Date: Tuesday, January 24, 2017 at 7:10 PM
To: James Sun mailto:james...@fb.com>>
Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
inheritance-shadow-warning-v0.9.patch
Description: inheritance-shadow-warning-v0.9.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Add warning for c++ member variable shadowing
se.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
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 mailto:compn...@compnerd.org>>
Date: Tuesday, January 24, 2017 at 7:10 PM
To: James Sun mailto:james...@fb.com>>
Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
inheritance-shadow-warning-v1.0.patch
Description: inheritance-shadow-warning-v1.0.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Add warning for c++ member variable shadowing
+ /// 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
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 mailto:compn...@compnerd.org>>
Date: Tuesday, January 24, 2017 at 7:10 PM
To: James Sun mailto:james...@fb.com>>
Cc: "cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>"
mailto:cfe-commits@lists.llvm.org>>, Aaron Ballman
mailto:aa...@aaronballman.com>>, Richard Smith
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(CurContext))
CheckShadowInheritedVariabless(Loc, Name.getAsString(), RD, RD);
On Tue, Jan 24, 2017 at 4:06 PM, James Sun via cfe-commits
mailto:cfe-commits@lists.llvm.org>> wrote:
Coding style change
From: James Sun 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>"
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
inheritance-shadow-warning-v1.1.patch
Description: inheritance-shadow-warning-v1.1.patch
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
