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

 
 

bug #48493: Clear up "shadowed member" warnings

Submitter:  Lachlan Andrew <lachlan>
Submitted:  Tue 12 Jul 2016 11:44:54 PM UTC
   
 
Category:  Configuration and Build System Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Other
Status:  Wont Fix Assigned to:  lachlan
Originator Name:  Lachlan Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group 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>
Group 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>
Group Member
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


class a
{
public:
  void (*callback) ();

  a (void (*default_callback) () )
  {
    if (default_callback == NULL)
      {
        // something long enough
        // to forget what parameter name I used
      }

    callback = default_callback;
  }

  void default_callback (void);
  void other_callback (void);
}

a  b(a::other_callback);


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>
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>
Group 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>
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>
Group Member
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>
Group Member
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>
Group 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>
Group 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>
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>

 

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

Attach Files:
   
   
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
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by lachlan (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-22 jwe StatusPatch Submitted Wont Fix
        Open/ClosedOpen Closed
    2016-07-13 lachlan Attached File- Added maint_shadowed_vars.cset, #37834
    2016-07-12 lachlan Attached File- Added maint_shadowed_vars.cset, #37833

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code