bugGNU Octave - Bugs: bug #55741, build with '-Woverloaded-virtual'...

 
 

bug #55741: build with '-Woverloaded-virtual' emits many warnings indicating problems

Submitter:  Mike Miller <mtmiller>
Submitted:  Wed 20 Feb 2019 03:10:22 AM UTC
   
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  Fixed Assigned to:  None
Originator Name:  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 15 Mar 2019 07:28:08 PM UTC, comment #16: 

I added -Woverloaded-virtual to the list of extra C++ compiler warning options here:

http://hg.savannah.gnu.org/hgweb/octave/rev/7fdbb03d5f76

John W. Eaton <jwe>
Group administrator
Fri 15 Mar 2019 06:21:13 PM UTC, comment #15: 

Build looks very clean to me, closing as fixed.

Mike Miller <mtmiller>
Group Member
Fri 15 Mar 2019 07:27:28 AM UTC, comment #14: 
John W. Eaton <jwe>
Group administrator
Fri 15 Mar 2019 06:02:43 AM UTC, comment #13: 

I pushed two more changesets for this problem (subsref and call methods):

http://hg.savannah.gnu.org/hgweb/octave/rev/9cd4b045fe3d
http://hg.savannah.gnu.org/hgweb/octave/rev/9dd1d8973877

John W. Eaton <jwe>
Group administrator
Thu 14 Mar 2019 06:23:12 PM UTC, comment #12: 


> Is one of those builds GCC and the other Clang?


Both are clang.

I think what's happening is that the Homebrew brew environment is turning off warnings: I don't see any of the other warnings listed here in the brewed octave build logs. That's probably it; not something on Octave's end.

Andrew Janke <apjanke>
Thu 14 Mar 2019 06:19:25 PM UTC, comment #11: 


> I pushed the following change for the stack_frame varval and varref warnings


Thank you for making an exception in this case. The build log is much cleaner now.

Andrew Janke <apjanke>
Thu 14 Mar 2019 06:17:48 PM UTC, comment #10: 

@apjanke

> Anybody know why this would be, if -Wall/-Woverloaded-virtual isn't supposed to be the default?


Is one of those builds GCC and the other Clang?

Octave does use -Wall by default, but 'gcc -Wall' does not enable -Woverloaded-virtual, while 'clang -Wall' does.

Mike Miller <mtmiller>
Group Member
Thu 14 Mar 2019 05:59:01 PM UTC, comment #9: 

I pushed the following change for the stack_frame varval and varref warnings:

http://hg.savannah.gnu.org/hgweb/octave/rev/b54d9581b1ac

Changes like this are OK, but we should examine each case individually to decide whether a using decl is really the best solution so I'm not just doing this for all the warnings at once.

John W. Eaton <jwe>
Group administrator
Thu 14 Mar 2019 10:21:03 AM UTC, comment #8: 

A few more warnings found under clang on macOS 10.14.3 with Xcode 10.1:

A C linkage issue in oct-sparse:


In file included from ../../octave/liboctave/util/oct-sparse.h:75:
/usr/local/include/cs.h:442:14: warning: 'cs_ci_house' has C-linkage specified, but returns user-defined type 'cs_complex_t' (aka 'complex<double>') which is incompatible with C [-Wreturn-type-c-linkage]
cs_complex_t cs_ci_house (cs_complex_t *x, double *beta, int n) ;
             ^
/usr/local/include/cs.h:582:14: warning: 'cs_cl_house' has C-linkage specified, but returns user-defined type 'cs_complex_t' (aka 'complex<double>') which is incompatible with C [-Wreturn-type-c-linkage]
cs_complex_t cs_cl_house (cs_complex_t *x, double *beta, cs_long_t n) ;
             ^


Non-prototype function declarations pulled in from readline:


n file included from /usr/local/opt/readline/include/readline/readline.h:36:
/usr/local/opt/readline/include/readline/rltypedefs.h:35:22: warning: this function declaration is not a prototype [-Wstrict-prototypes]
typedef int Function () __attribute__ ((deprecated));
                     ^
                      void
/usr/local/opt/readline/include/readline/rltypedefs.h:36:24: warning: this function declaration is not a prototype [-Wstrict-prototypes]
typedef void VFunction () __attribute__ ((deprecated));
                       ^
                        void
/usr/local/opt/readline/include/readline/rltypedefs.h:37:26: warning: this function declaration is not a prototype [-Wstrict-prototypes]
typedef char *CPFunction () __attribute__ ((deprecated));
                         ^
                          void
/usr/local/opt/readline/include/readline/rltypedefs.h:38:28: warning: this function declaration is not a prototype [-Wstrict-prototypes]
typedef char **CPPFunction () __attribute__ ((deprecated));
                           ^
                            void
In file included from ../../octave/liboctave/util/oct-rl-edit.c:32:
/usr/local/opt/readline/include/readline/readline.h:398:23: warning: this function declaration is not a prototype [-Wstrict-prototypes]
extern int rl_message ();
                      ^
                       void


Some old-style casts:


../../octave/libinterp/dldfcn/__delaunayn__.cc:215:15: warning: use of old-style cast [-Wold-style-cast]
              FOREACHvertex_ (facet->vertices)
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/include/libqhull/libqhull.h:950:34: note: expanded from macro 'FOREACHvertex_'
#define FOREACHvertex_(vertices) FOREACHsetelement_(vertexT, vertices,vertex)
                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/include/libqhull/qset.h:139:24: note: expanded from macro 'FOREACHsetelement_'
          variable##p= (type **)&((set)->e[0].p); \
                       ^        ~~~~~~~~~~~~~~~~


Some old style casts that look like they're pulled in from the portaudio library:


../../octave/libinterp/dldfcn/audiodevinfo.cc:59:12: warning: use of old-style cast [-Wold-style-cast]
    return paInt8;
           ^~~~~~
/usr/local/Cellar/portaudio/19.6.0/include/portaudio.h:491:27: note: expanded from macro 'paInt8'
#define paInt8           ((PaSampleFormat) 0x00000010) /**< @see PaSampleFormat */
                          ^                ~~~~~~~~~~
../../octave/libinterp/dldfcn/audiodevinfo.cc:61:12: warning: use of old-style cast [-Wold-style-cast]
    return paInt16;
           ^~~~~~~
/usr/local/Cellar/portaudio/19.6.0/include/portaudio.h:490:27: note: expanded from macro 'paInt16'
#define paInt16          ((PaSampleFormat) 0x00000008) /**< @see PaSampleFormat */
                          ^                ~~~~~~~~~~
../../octave/libinterp/dldfcn/audiodevinfo.cc:63:12: warning: use of old-style cast [-Wold-style-cast]
    return paInt24;
           ^~~~~~~
/usr/local/Cellar/portaudio/19.6.0/include/portaudio.h:489:27: note: expanded from macro 'paInt24'
#define paInt24          ((PaSampleFormat) 0x00000004) /**< Packed 24 bit format. @see PaSampleFormat */
                          ^                ~~~~~~~~~~
../../octave/libinterp/dldfcn/audiodevinfo.cc:65:12: warning: use of old-style cast [-Wold-style-cast]
    return paInt32;
           ^~~~~~~
/usr/local/Cellar/portaudio/19.6.0/include/portaudio.h:488:27: note: expanded from macro 'paInt32'
#define paInt32          ((PaSampleFormat) 0x00000002) /**< @see PaSampleFormat */
                          ^                ~~~~~~~~~~
../../octave/libinterp/dldfcn/audiodevinfo.cc:67:12: warning: use of old-style cast [-Wold-style-cast]
    return paFloat32;
           ^~~~~~~~~
/usr/local/Cellar/portaudio/19.6.0/include/portaudio.h:487:27: note: expanded from macro 'paFloat32'
#define paFloat32        ((PaSampleFormat) 0x00000001) /**< @see PaSampleFormat */
                          ^                ~~~~~~~~~~
  CXX      libinterp/dldfcn/colamd_la-colamd.lo


Andrew Janke <apjanke>
Thu 14 Mar 2019 10:11:15 AM UTC, comment #7: 

Maybe the `octave::scope_stack_frame::varval` and `octave::scope_stack_frame::varref` overrides could be prioritized? They're used all over the place, so the warnings for them really spam the build log.

Andrew Janke <apjanke>
Thu 14 Mar 2019 10:06:26 AM UTC, comment #6: 

I'm seeing a couple additional warnings with clang on macOS 10.14.3 / Xcode 10.1.


In file included from ../../octave/libinterp/corefcn/fcn-info.h:36:
../../octave/libinterp/corefcn/symscope.h:120:16: warning: expression result unused [-Wunused-value]
      new_sid->m_name;
      ~~~~~~~  ^~~~~~
[...]
In file included from ../../octave/libgui/src/octave-qt-link.h:37:
../../octave/libgui/src/octave-gui.h:54:9: warning: private field 'm_argc' is not used [-Wunused-private-field]
    int m_argc;
        ^
../../octave/libgui/src/octave-gui.h:55:12: warning: private field 'm_argv' is not used [-Wunused-private-field]
    char **m_argv;
           ^
2 warnings generated.


These look like things that should actually be fixed.

I can't tell if that `new_sid->m_name;` should actually be `new_sid->m_name = m_name;` or not.

Looks like the unused private fields can, by definition, be removed.

Andrew Janke <apjanke>
Thu 14 Mar 2019 10:02:26 AM UTC, comment #5: 

For some reason, I'm always seeing these warnings when building from default. It looks like my build is using -Wall by default. Anybody know why this would be, if -Wall/-Woverloaded-virtual isn't supposed to be the default?

This is the configure I'm using:


../../octave/configure \
  --prefix=/tmp/test-octave \
  --without-x \
  --disable-no-undefined \
  --disable-docs \
  --without-fltk \
  --with-qt=5 \
  --with-hdf5-includedir=$brew_opt/hdf5/include \
  --with-hdf5-libdir=$brew_opt/hdf5/lib \
  --with-blas="-L${brew_opt}/veclibfort/lib -lvecLibFort" \
  | tee config.out


And in the generated Makefile, I see this:


WARN_CFLAGS =  -Wall -W -Wshadow -Wformat -Wpointer-arith -Wmissing-prototypes -Wstrict-prototypes -Wwrite-strings -Wcast-align -Wcast-qual
WARN_CXXFLAGS =  -Wall -W -Wshadow -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual


This is on macOS 10.14.3.

For some reason, I don't see these warnings when building using a Homebrew formula, even though it uses a similar configure:


./configure
--prefix=/usr/local/Cellar/octave-default/default
--disable-dependency-tracking
--disable-silent-rules
--enable-link-all-dependencies
--enable-shared
--disable-static
--without-fltk
--with-hdf5-includedir=/usr/local/opt/hdf5/include
--with-hdf5-libdir=/usr/local/opt/hdf5/lib
--with-x=no
--with-blas=-L/usr/local/opt/veclibfort/lib -lvecLibFort
--with-portaudio
--with-sndfile
--with-qt=5


Andrew Janke <apjanke>
Wed 20 Feb 2019 06:41:42 PM UTC, comment #4: 

Yeah, that's what I was thinking, we need a case by case look at each function, some of them may not need to be overridden and so we can safely remove the 'virtual' on the overload to silence the warning.

Mike Miller <mtmiller>
Group Member
Wed 20 Feb 2019 03:07:32 PM UTC, comment #3: 

After thinking about this some more, I still agree that we should fix these to avoid possible confusion.  But I don't think it's anything that needs to be fixed immediately.  This warning is saying that a base class virtual function is hidden by another function of the same name but different signature in the derived class.  If these derived classes needed the base class function, we would see an error about it not being available.  So it may be OK that they are hidden.  But the overloaded names may also cause some future confusion.  It depends on the details for each case.  So we shouldn't just go and add public "using" declarations (or similar) in each of these derived classes without first thinking about whether it is intentional that the base class functions are hidden or whether there should be some other solution.  Maybe some of them don't need to be virtual?

John W. Eaton <jwe>
Group administrator
Wed 20 Feb 2019 02:20:27 PM UTC, comment #2: 

Yes, these need to be fixed and I also think we should always be using this warning option so that we are less likely to introduce any new overloads like these.

John W. Eaton <jwe>
Group administrator
Wed 20 Feb 2019 07:03:49 AM UTC, comment #1: 

Here is a list of virtual class member functions that are hidden by non-virtual overload functions of the same name in a derived class.

  • base_graphics_object::get(bool) const
  • octave::base_value_stack_frame::varref(size_t)
  • octave::base_value_stack_frame::varval(size_t) const
  • octave::opengl_renderer::draw(const Matrix&, bool)
  • octave::stack_frame::varref(const octave::symbol_record&)
  • octave::stack_frame::varref(size_t)
  • octave::stack_frame::varval(const octave::symbol_record&) const
  • octave::stack_frame::varval(size_t) const
  • octave_base_matrix<MT>::diag(octave_idx_type, octave_idx_type) const
  • octave_base_scalar<ST>::diag(octave_idx_type) const
  • octave_base_value::assign(const string&, const octave_value&)
  • octave_base_value::diag(octave_idx_type, octave_idx_type) const
  • octave_base_value::numel() const
  • octave_base_value::numel(const octave_value_list&)
  • octave_base_value::subsref(const string&, const std::list<octave_value_list>&)
  • octave_base_value::subsref(const string&, const std::list<octave_value_list>&, bool)


The list of member functions that are overloading these virtual functions is a bit longer, will take some more time to go through and make this useful. But as one simple example, octave::opengl_selector is a derived class of octave::opengl_renderer, and it defines 'virtual void draw (const graphics_object&, bool)' only.

Mike Miller <mtmiller>
Group Member
Wed 20 Feb 2019 03:10:22 AM UTC, original submission:  

Building the default branch with the warning option '-Woverloaded-virtual' emits many warnings, see attached log file produced by GCC 8.

These should be fixed, because the warning indicates that a non-virtual function in a derived class may be accidentally shadowing a virtual function with the same name, breaking the intended virtual function dispatching.

Should we also add '-Woverloaded-virtual' to the standard set of warnings to make sure these are fixed and don't crop up again?

I will try to find some time to go through the raw log and list the specific class member functions that are problems, many of the warnings are duplicated each time a header file is included.

@cdf - feel free to attach any additional warnings you saw with Clang if they aren't covered by the GCC warnings in my log file.

Inspired by https://lists.gnu.org/archive/html/octave-maintainers/2019-02/msg00120.html.

Mike Miller <mtmiller>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #46308:  Woverloaded-virtual.log.gz added by mtmiller (87KiB - application/gzip)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by apjanke (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller
  •  

    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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-15 mtmiller StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2019-03-15 jwe StatusNone Ready For Test
    2019-02-20 mtmiller Carbon-CopyRemoved 80942 -
    2019-02-20 mtmiller Attached File- Added Woverloaded-virtual.log.gz, #46308
        Carbon-Copy- Added cdf

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code