bugGNU Octave - Bugs: bug #48505, build does not use any warning...

 
 

bug #48505: build does not use any warning options in libgui/qterminal

Submitted by:  Mike Miller <mtmiller>
Submitted on:  Wed 13 Jul 2016 10:29:04 PM UTC  
 
Category:  Configuration and Build System Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Any

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)

Fri 13 Jul 2018 03:32:33 PM UTC, comment #15:

No problems. I was solving it in the most expedient manner, not necessarily the best. After updating, runing 'make' failed. I eventually did a 'make maintainer-clean' followed by a full bootsrap, configure, and make. The build system seems fine now.

Rik <rik5>
Project Administrator
Fri 13 Jul 2018 07:06:08 AM UTC, comment #14:

Rik: Thanks, that patch will work, but I thought we should use the prototype for xerbla that is in lo-blas-proto.h instead of repeating it in xerbla.c. So I included lo-blas-proto.h there and immediately found that it will only work with C++ because of our use of references instead of pointers. I don't want to give that up, or complicate things by using references for C++ code and pointers for C code, so I ended up converting xerbla to C++. Then the prototypes aren't needed. Here's my change (accidentally pushed as two changesets when I should have merged them before pushing):

http://hg.savannah.gnu.org/hgweb/octave/rev/9ea68b9e277b
http://hg.savannah.gnu.org/hgweb/octave/rev/a8325f2361c7

John W. Eaton <jwe>
Project Administrator
Fri 13 Jul 2018 05:37:32 AM UTC, comment #13:

Okay, that's good then.

I silenced the newly emitted warnings by adding prototypes to xerbla.c immediately above the function declaration. This seems redundant, but it works. See https://hg.savannah.gnu.org/hgweb/octave/rev/fcfedeff926c.

Rik <rik5>
Project Administrator
Fri 13 Jul 2018 02:36:13 AM UTC, comment #12:

I have carefully gone over the diffs, thanks for going in depth to compare the build log, but I don't see any problems here.

The changes in your diff are the following

  • display-available.c and xerbla.c are now being built with the same set of warnings options as the rest of Octave, they were being built with no warnings options before. This seems like a good change.
  • _fltk_uigetfile_.cc, _init_fltk_.cc, and _init_gnuplot_.cc are now being compiled with fewer redundant -I options. All of the -I options that were there before are still there. I think this is also a good change. If it breaks something for someone we can fix it by cleaning up or reordering the CPPFLAGS variables.

As an example from your log file, _init_gnuplot_.cc used to be built with the following -I options, broken out for clarity, and omitting Octave internal -I options:

where the '...' indicates other compiler options. Now it builds with

So it has dropped the number of '-I/usr/include/freetype2' options from four to two. And the reason for still having it twice is because gnuplot includes '$(FT2_CPPFLAGS) $(FONTCONFIG_CPPFLAGS)' in its options, both of which include '-I/usr/include/freetype2' (since Fontconfig depends on FreeType).

The same goes for the fltk source files, it has reduced redundant -I options but nothing has been lost.

Mike Miller <mtmiller>
Project Administrator
Fri 13 Jul 2018 01:31:23 AM UTC, comment #11:

I did a sanity check on the compilations by comparing the log files of the make process.

There are a few differences that I would have you look at. The file is attached as diffs.to_inspect.

The first difference I see is that we are now including the WARN_FLAGS when compiling in liboctave. For example,

This is probably okay. However, it has produced two new warnings during compilation.

The last area that needs inspection are three files in dldfcn. Looking just at _init_gnuplot_.cc I see during the link

The old compile was including "-I/usr/include/freetype2" twice. Twice seems excessive, but once might be necessary if gnuplot really does depend on FreeType.

(file #44546)

Rik <rik5>
Project Administrator
Thu 12 Jul 2018 11:47:28 PM UTC, comment #10:

And a couple more simplifications

https://hg.savannah.gnu.org/hgweb/octave/rev/e65dc7a2de42
https://hg.savannah.gnu.org/hgweb/octave/rev/6e6b5e85e08a

We now just have project-wide AM_CFLAGS, AM_CXXFLAGS, and AM_FFLAGS defined once, with the few exceptions defined in their own module.mk files (libgui/qterminal, liboctave/external, and liboctave/util).

Thanks for the help!

Mike Miller <mtmiller>
Project Administrator
Thu 12 Jul 2018 11:13:26 PM UTC, comment #9:

Yes, thanks, I pushed a change to fix that oversight

https://hg.savannah.gnu.org/hgweb/octave/rev/12bb7bd389fc

Mike Miller <mtmiller>
Project Administrator
Thu 12 Jul 2018 10:31:34 PM UTC, comment #8:

I think there might need to be an adjustment for libinterp/dldfcn/module.mk. As an example,

It used to be that libinterp/module.mk defined libinterp_liboctinterp_la_CXXFLAGS:

but it no longer does.

Can the variable just be deleted entirely or should it now contain $(AM_CXXFLAGS)?

Rik <rik5>
Project Administrator
Thu 12 Jul 2018 10:08:23 PM UTC, comment #7:

The diff looks like the essence of what I wanted to accomplish, so I'm closing this as fixed and leaving it to the buildbots to test it.

Mike Miller <mtmiller>
Project Administrator
Thu 12 Jul 2018 09:34:58 PM UTC, comment #6:

I pushed the following change:

http://hg.savannah.gnu.org/hgweb/octave/rev/90d43011e8e7

Mike or Rik: If this looks OK to you, please close this report.

John W. Eaton <jwe>
Project Administrator
Thu 12 Jul 2018 09:14:15 PM UTC, comment #5:

The patches don't apply now, but I'm looking at it and will check something in soon.

John W. Eaton <jwe>
Project Administrator
Thu 12 Jul 2018 08:19:37 PM UTC, comment #4:

Yeah I think we should apply both file #37865 and file #37877 as one a single change. I haven't tested these changes in a long while, not sure if they still apply or are complete.

Mike Miller <mtmiller>
Project Administrator
Thu 12 Jul 2018 04:26:41 PM UTC, comment #3:

Just noting, a patch was submitted by jwe based on Mike's work back in 2016, but never seems to have made it to the "Patch Reviewed" phase.

I agree with jwe that we really don't want to enable warnings on the qterminal directory. First, it is someone else's code and debugging and understanding someone else's code enough to decide whether a warning is real or not is a PITA. Secondly, jwe has talked about removing this stuff entirely, so there's no reason to spend a lot of time on it.

Rik <rik5>
Project Administrator
Thu 14 Jul 2016 09:20:42 PM UTC, comment #2:

Simplifying the way we handle the compiler flags would be good. I'd prefer to not enable warnings in the qterminal code though. So how about applying your changes except that in the /libgui/qterminal-module.mk file, use the attached change?

(file #37877)

John W. Eaton <jwe>
Project Administrator
Wed 13 Jul 2016 10:31:23 PM UTC, comment #1:

And here's what I have changed so far, which simplifies all the compiler flags defined throughout the project but causes all these warnings to be emitted.

(file #37865)

Mike Miller <mtmiller>
Project Administrator
Wed 13 Jul 2016 10:29:04 PM UTC, original submission:

Octave's configure script sets some default compiler options in the variables $(CXXFLAGS), $(AM_CXXFLAGS), and $(XTRA_CXXFLAGS), as well as some gcc warning options in the variable $(WARN_CXXFLAGS).

The top-level Makefile.am does

and then every subdirectory does its own

This seems pretty redundant and wasteful, so I tried to simplify this by just adding the WARN_ flags to the AM_ flags at the top and eliminating all of the unnecessary foo_CXXFLAGS. The end result is that all the flags end up being the same, but we are intentionally omitting warning flags from the libgui/qterminal subdirectory.

The qterminal code (that we imported from somewhere else) contains many many warnings that our default flags would catch, including

  • 836 counts of -Wold-style-cast
  • 15 counts of -Wunused-variable
  • 7 counts of -Wcast-qual
  • 6 counts of -Wshadow
  • 1 count of -Wunused-parameter

This is not a huge priority, and maybe we leave it as it is now, but good to remember that there is potentially buggy code in the qterminal directory, and maybe we can fix it or maybe we replace the entire command window implementation before that happens.

Mike Miller <mtmiller>
Project Administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #44546:  diffs.to_inspect added by rik5 (22KiB - application/octet-stream)
file #37877:  diffs.txt added by jwe (530B - text/plain)
file #37865:  cflags-simplify.diff added by mtmiller (10KiB - 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 rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by mtmiller (Submitted the item)
  •  

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

    Only project members can vote.

     

     

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-07-13 rik5 Attached File- => Added diffs.to_inspect, #44546
    2018-07-12 mtmiller StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2018-07-12 jwe StatusPatch Submitted => Ready For Test
    2018-07-12 rik5 StatusConfirmed => Patch Submitted
    2016-07-14 jwe Attached File- => Added diffs.txt, #37877
    2016-07-13 mtmiller Attached File- => Added cflags-simplify.diff, #37865

    Back to the top


    Powered by Savane 3.3