[cfe-dev] Patch: Re: -Wshadow rationale for data members

[John McCall]

On Nov 9, 2010, at 11:24 AM, Adam Nohejl wrote:

> John,
> 
> Thank you very much for a thorough review!

No problem.  Please reply to cfe-dev, though, just so other people can chip in.

>> If you're in a CXXMethodDecl, you're in a C++ translation unit, so all RecordDecls will be CXXRecordDecls;  [...]
> 
> Thanks for clarification, wasn't sure about that.
> 
>> The preferred way to get the innermost function is getCurFunctionOrMethodDecl(), [...]
> 
> OK.
> 
>> You could avoid a lot of these cast<>s if you used dyn_cast instead of isa, [...]
> 
> OK.
> 
>> Also, please include your test cases as part of your next patch.
> 
> OK.
> 
> As for the decision what should be warned about, I will think it once over and possibly rewrite the code.
> 
> I mostly agreee with what you have written, but here's a few questions:
> 
>> - we shouldn't warn about shadowing class members, or when class members shadow non-class members; otherwise
> 
> Why not? Is the rationale that if needed you can still access class members by additional qualification?(You could apply that to base class instance variables too.)

Well, we definitely need to be more careful about warning about shadowing class members.  Often you get class member functions with very generic names that aren't necessarily a problem to shadow.  But I'd be fine with you experimenting with finer-grained rules here.

I do think we definitely want to not warn about shadowing outer class members with inner class members, though.

>>> class X{
>>>  int a;
>>>  class X{
>>>      enum{ a = 0 };                // (4) no warning (OK)
>>>      X(int a){ }                   // (5) missing warning? (in contrast to gcc)
>> 
>> This definitely shouldn't warn.
> 
> Why not? Would you warn if "a" was a "const int", not an enum constant?

I think I missed that 'a' was shadowing the enum, not the outer member.  This is okay to warn about.

John.