bugGNU Octave - Bugs: bug #48493, Clear up "shadowed...

 
 

bug #48493: Clear up "shadowed member" warnings

Submitted by:  Lachlan Andrew <lachlan>
Submitted on:  Tue 12 Jul 2016 11:44:54 PM UTC  
 
Category: Configuration and Build SystemSeverity: 2 - Minor
Priority: 5 - NormalItem Group: Other
Status: Wont FixAssigned to: Lachlan Andrew <lachlan>
Originator Name: LachlanOpen/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 22 Jul 2016 05:10:23 PM UTC, comment #11:

I'm closing this report as won't fix. If the warnings from GCC 4 and earlier cause too much noise, you can always remove the -Wshadow option from the compiler flags. Or move to GCC 5 or newer.

John W. Eaton <jwe>
Project Administrator
Sun 17 Jul 2016 10:54:08 AM UTC, comment #10:

I agree that function pointer names that conflict with member function names would be a problem so I'm glad that GCC will still warn about those.

But for ordinary parameter names, it is a hassle, because one often wants a member variable named FOO that with an accessor function (FOO seems like an obvious name for that) and that may be passed as a construtor argument (FOO is also an obvious choice for that name). I'm willing to use m_FOO for the member variable name. I have used FOO_arg or xFOO in the past for parameter names, but only because GCC was warning about it. I'm happy to not have to do that.

John W. Eaton <jwe>
Project Administrator
Sun 17 Jul 2016 04:45:03 AM UTC, comment #9:

I think jwe is saying that we should start instituting a policy of trying to name all class member variables "m_foo" to avoid ambiguity with public member functions or other names.

The patch that you have submitted here is all about renaming local variables and parameter names that shadow class member functions. In comment #7 I believe jwe is saying that he doesn't think these types of naming conflicts are problems worth fixing. And the GCC developers apparently agree which is why they have stopped emitting -Wshadow for exactly these situations.

Your example concerns a parameter of type function pointer. This is still caught by the -Wshadow warning in GCC 6. If you look at the bug report I linked, GCC still warns for variables that are function pointers. The most common case of a simple variable that has the same name as a member function should not be a problem.

Mike Miller <mtmiller>
Project Administrator
Sat 16 Jul 2016 02:52:23 AM UTC, comment #8:

JWE, I'm still not quite sure what you want done.

Let's use the example of "exit_status" from octave_exit_exception::octave_exit_exception(int, bool).

That class currently has a private member variable m_exit_status, a public function exit_status, and (as if by accident) a parameter exit_status.

We can't rename the function m_exit_status, because that conflicts with the member variable name.

We can make a potentially large change, and rename the member function to something like get_exit_status, or we can make a small change to fix the "accident" and rename the parameter, to something like p_exit_status.

I'm happy to do whichever you suggest (except the impossible one, which is how I interpret your previous requests), but will argue below that changing the parameter is the best approach.

First, I don't see why it is not a problem to have a parameter named the same as a member function. (I know this is something we cannot change...) Consider the code

That is unambiguous to the compiler (which is why it is not an error), but misleading (which is why it should be a warning).

We don't even need to have function-type variables. Simply being told that a call "exit_status ()" is invalid is misleading.

Second, if devs are using a version of gcc that doesn't warn of situations like the above, then they are likely to create new constructors for existing, possibly widespread, classes that cause this ambiguity. A global policy of changing the member name would require widespread changes to the code, to essentially work around an accident. (That doesn't mean we can't do it in this particular case.)

Oh, and "_m" was just a typo. Sorry for any confusion.

Lachlan Andrew <lachlan>
Project MemberIn charge of this item.
Thu 14 Jul 2016 11:17:15 AM UTC, comment #7:

Oh, now I see that the naming convention I proposed doesn't help with the case of, for example, a constructor argument called "exit_status" and a member function with the same name. But that's not really a problem, which is why the GCC maintainers dropped that warning. OTOH, a function argument or a local variable name that shadows a member variable is still flagged.

As for leading "_" vs "m_", I would prefer "m_" in Octave sources please (not "_m").

John W. Eaton <jwe>
Project Administrator
Wed 13 Jul 2016 11:40:40 PM UTC, comment #6:

Thanks for the digging, Mike.

I personally think GCC 5's fix is a backward step, so it isn't clear that it won't revert to the more verbose behaviour.

I like JWE's suggestion of having consistent naming of members. The Qt code already consistently uses a "_" prefix. I'd be inclined to do that, which I think is a common Qt standard, rather than having a mix of "_" and "_m".

However, making this consistent would be a major refactor that should probably be done after all patches that will go into 4.2.0 have been applied.

Should I mark this as "Won't Do" (until after 4.2.0)?

Lachlan Andrew <lachlan>
Project MemberIn charge of this item.
Wed 13 Jul 2016 08:33:42 PM UTC, comment #5:

So while the patch may be worth applying anyway (with the naming suggestions made by jwe), I don't think there's any way to actually reproduce the warnings you are seeing with the modern versions of GCC that some of us are using. As time goes on and more people upgrade to GCC 5 and 6, similar constructs may return and go unnoticed and maybe that's ok, if the compiler can't tell us about them then we can't very easily detect them.

Mike Miller <mtmiller>
Project Administrator
Wed 13 Jul 2016 08:29:29 PM UTC, comment #4:

I did some digging. Some of these may be false positives with older versions of gcc. GCC 5 has "fixed" the -Wshadow option by making it not warn about variables and methods that have the same name, only class variables and local variables or parameters.

So for example having a local variable named "parent" no longer emits a -Wshadow warning if the class or its base class has a method also named "parent".

Reference to GCC bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57709

Mike Miller <mtmiller>
Project Administrator
Wed 13 Jul 2016 08:26:22 PM UTC, comment #3:

Also, rather than using names like parnt, I'd prefer to move in the direction of having member variables prefixed with "m_" where possible.

John W. Eaton <jwe>
Project Administrator
Wed 13 Jul 2016 08:25:39 PM UTC, comment #2:

I don't see the warnings. Can you post a compile log that shows them? What compiler (and version) are you using? Also, for one of the files that generates a warning, can you show the command that you are using? "make V=1 .." Thanks.

John W. Eaton <jwe>
Project Administrator
Wed 13 Jul 2016 03:20:26 AM UTC, comment #1:

Oops. The last patch missed a couple. Try this one.

(file #37834)

Lachlan Andrew <lachlan>
Project MemberIn charge of this item.
Tue 12 Jul 2016 11:44:54 PM UTC, original submission:

The attached patch renames some local variables that shadow member variables or methods.

(I'm puzzled by the ToggleButtonControl one, since it doesn't seem to have a member named parent, but presumably a base class does.)

I still get warnings about xmin and xmax being deprecated, but don't know how to fix those. It requires passing a function template, but the obvious argument octave::math::max<T> gives an error.

Lachlan Andrew <lachlan>
Project MemberIn charge of this item.

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #37834:  maint_shadowed_vars.cset added by lachlan (6KiB - text/x-diff)
file #37833:  maint_shadowed_vars.cset added by lachlan (5KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by lachlan (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 22 Jul 2016 05:10:23 PM UTCjweStatusPatch Submitted=>Wont Fix
      Open/ClosedOpen=>Closed
    Wed 13 Jul 2016 03:20:26 AM UTClachlanAttached File-=>Added maint_shadowed_vars.cset, #37834
    Tue 12 Jul 2016 11:44:54 PM UTClachlanAttached File-=>Added maint_shadowed_vars.cset, #37833

    Back to the top


    Powered by Savane 3.1-cleanup1