review of compiler warnings

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

review of compiler warnings

Jim Newton
I'm using xcode on the Mac when compiling musescore, and it issues lots of warnings.
Looking at these, my opinion is that many (if not most) should be fixed.

Here is an example.  Indeed, the "return; // no parts read" seems really wrong to me.

file importmxmlpass1.cpp, lin 582
Warning: Logical is only applied to the left hand side of this comparison.

static void determineMeasureStart(const QVector<Fraction>& ml, QVector<Fraction>& ms)
      {
      ms.resize(ml.size());
      if (!ms.size() > 0)
            return;  // no parts read

      // first measure starts at t = 0
      ms[0] = Fraction(0, 1);
      // all others start at start time previous measure plus length previous measure
      for (int i = 1; i < ml.size(); i++)
            ms[i] = ms.at(i - 1) + ml.at(i - 1);
      //for (int i = 0; i < ms.size(); i++)
      //      qDebug("measurestart ms[%d] %s", i + 1, qPrintable(ms.at(i).print()));
      }
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Jojo-Schmitz
Looks like a very valid warning to me, strange that there is no such warning
in the Windows build

Just dropping the "> 0" part should fix it, I don't think ms.size() can ever
return something < 0.

-----Original Message-----
From: Jim Newton [mailto:[hidden email]]
Sent: Tuesday, May 05, 2015 10:26 AM
To: [hidden email]
Subject: [Mscore-developer] review of compiler warnings

I'm using xcode on the Mac when compiling musescore, and it issues lots of
warnings.
Looking at these, my opinion is that many (if not most) should be fixed.

Here is an example.  Indeed, the "return; // no parts read" seems really
wrong to me.

file importmxmlpass1.cpp, lin 582
Warning: Logical is only applied to the left hand side of this comparison.

static void determineMeasureStart(const QVector<Fraction>& ml,
QVector<Fraction>& ms)
      {
      ms.resize(ml.size());
      if (!ms.size() > 0)
            return;  // no parts read

      // first measure starts at t = 0
      ms[0] = Fraction(0, 1);
      // all others start at start time previous measure plus length
previous measure
      for (int i = 1; i < ml.size(); i++)
            ms[i] = ms.at(i - 1) + ml.at(i - 1);
      //for (int i = 0; i < ms.size(); i++)
      //      qDebug("measurestart ms[%d] %s", i + 1,
qPrintable(ms.at(i).print()));
      }



--
View this message in context:
http://dev-list.musescore.org/review-of-compiler-warnings-tp7579292.html
Sent from the MuseScore Developer mailing list archive at Nabble.com.

----------------------------------------------------------------------------
--
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications Performance
metrics, stats and reports that give you Actionable Insights Deep dive
visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Leon Vinken
A few remarks:

- the return is intended and correct: if ml is empty, the remainder of the function cannot (and should not) be executed.
- the test is obviously a typo, thanks for catching that, it was meant to be "if (!(ms.size() > 0))" (note extra pair of parentheses)
- due to a few lucky coincidences, the code as is ("if (!ms.size() > 0)") executes correctly (size = 0 is interpreted as false, !false evaluates to "1" in an integer context, note ! has precedence over >)
- "if (ms.isEmpty())" would be the most concise rewrite
- my Xcode version (4.6.3) does not warn me for this error, otherwise I would have fixed it
- I would very much like MuseScore to compile without warnings on Xcode, simply because the current amount of warnings easily mask serious problems, but this seems a lot of work. If clang is more strict than gcc (used by Werner), Xcode warnings will keep creeping in :-(.
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Jojo-Schmitz
I believe getting rid of (read: disable) the warning ""...hides overloaded
virtual function" should help a lot here. Guess it might be as simple as
using "-Wno-overloaded-virtual"

-----Original Message-----
From: Leon Vinken [mailto:[hidden email]]
Sent: Tuesday, May 05, 2015 1:15 PM
To: [hidden email]
Subject: Re: [Mscore-developer] review of compiler warnings

A few remarks:

- the return is intended and correct: if ml is empty, the remainder of the
function cannot (and should not) be executed.
- the test is obviously a typo, thanks for catching that, it was meant to be
"if (!(ms.size() > 0))" (note extra pair of parentheses)
- due to a few lucky coincidences, the code as is ("if (!ms.size() > 0)")
executes correctly (size = 0 is interpreted as false, !false evaluates to
"1" in an integer context, note ! has precedence over >)
- "if (ms.isEmpty())" would be the most concise rewrite
- my Xcode version (4.6.3) does not warn me for this error, otherwise I
would have fixed it
- I would very much like MuseScore to compile without warnings on Xcode,
simply because the current amount of warnings easily mask serious problems,
but this seems a lot of work. If clang is more strict than gcc (used by
Werner), Xcode warnings will keep creeping in :-(.




--
View this message in context:
http://dev-list.musescore.org/review-of-compiler-warnings-tp7579292p7579294.
html
Sent from the MuseScore Developer mailing list archive at Nabble.com.

----------------------------------------------------------------------------
--
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications Performance
metrics, stats and reports that give you Actionable Insights Deep dive
visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

lasconic
Administrator
I did my best to clean all the warnings without changing the behavior and maintaining short method names in https://github.com/musescore/MuseScore/commit/61eed114019e58e5206ab5bd489b5cdd71cab901

I hope the code will still compile on other platforms, and without warnings.

lasconic

2015-05-05 13:27 GMT+02:00 Joachim Schmitz <[hidden email]>:
I believe getting rid of (read: disable) the warning ""...hides overloaded
virtual function" should help a lot here. Guess it might be as simple as
using "-Wno-overloaded-virtual"

-----Original Message-----
From: Leon Vinken [mailto:[hidden email]]
Sent: Tuesday, May 05, 2015 1:15 PM
To: [hidden email]
Subject: Re: [Mscore-developer] review of compiler warnings

A few remarks:

- the return is intended and correct: if ml is empty, the remainder of the
function cannot (and should not) be executed.
- the test is obviously a typo, thanks for catching that, it was meant to be
"if (!(ms.size() > 0))" (note extra pair of parentheses)
- due to a few lucky coincidences, the code as is ("if (!ms.size() > 0)")
executes correctly (size = 0 is interpreted as false, !false evaluates to
"1" in an integer context, note ! has precedence over >)
- "if (ms.isEmpty())" would be the most concise rewrite
- my Xcode version (4.6.3) does not warn me for this error, otherwise I
would have fixed it
- I would very much like MuseScore to compile without warnings on Xcode,
simply because the current amount of warnings easily mask serious problems,
but this seems a lot of work. If clang is more strict than gcc (used by
Werner), Xcode warnings will keep creeping in :-(.




--
View this message in context:
<a href="http://dev-list.musescore.org/review-of-compiler-warnings-tp7579292p7579294. html" target="_blank">http://dev-list.musescore.org/review-of-compiler-warnings-tp7579292p7579294.
html
Sent from the MuseScore Developer mailing list archive at Nabble.com.

----------------------------------------------------------------------------
--
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications Performance
metrics, stats and reports that give you Actionable Insights Deep dive
visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Jim Newton
In reply to this post by Jim Newton
Here is another warning which I don't understand.  Can someone help me understand it better.

Take a look at line 294 of file text.h:

      void writeProperties(Xml&, bool = true, bool = true) const;

My compiler claims: 'Ms::Text::writeProperties' hides overloaded virtual function

1) Until now, I haven't found any implementation of writeProperties with two boolean parameters, just this declaration.   Has someone found the implementation code?

2) What does this error mean?  Was exactly is being hidden?

Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

lasconic
Administrator
This should be solved in the last master. The two parameters have a default value. I solved it by removing the default value and create methods that this one overload.

Of course, the last master commit will fail to compile because of Werner's last changes http://dev-list.musescore.org/freetype-for-scorefonts-td7579297.html but except this it should be ok.

lasconic

2015-05-06 9:54 GMT+02:00 Jim Newton <[hidden email]>:
Here is another warning which I don't understand.  Can someone help me
understand it better.

Take a look at line 294 of file text.h:

      void writeProperties(Xml&, bool = true, bool = true) const;

My compiler claims: 'Ms::Text::writeProperties' hides overloaded virtual
function

1) Until now, I haven't found any implementation of writeProperties with two
boolean parameters, just this declaration.   Has someone found the
implementation code?

2) What does this error mean?  Was exactly is being hidden?





--
View this message in context: http://dev-list.musescore.org/review-of-compiler-warnings-tp7579292p7579298.html
Sent from the MuseScore Developer mailing list archive at Nabble.com.

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Jim Newton
In reply to this post by Jim Newton
Maybe I understand the issue.  Here are several declarations together.

      void writeProperties(Xml&, bool = true, bool = true) const;

Isn't the compiler warning me that if I call writeProperties with one Xml argument, that this method with two optional bools is called instead of the 1-arg version.

The compiler warning is saying that even if 1 argument is used at the call site, the 3-arg version will be called, and there is no way to reach the 3-arg version.

Thus if this class or a subclass makes a call like writeProperties(xmlobj), then 3-arg version is called and that might be surprising.
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Jim Newton
In reply to this post by lasconic
yes that was one of my suggested fixes.
The problem with that fix is you have to find all unary calls to this function, and decide which ones need a boolean true inserted into the call-site.
ABL
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

ABL
Since we are set to check all compiler warnings, here are the warnings that still appear for:

gcc5 under Linux Mint 17.1 http://kpaste.net/9d9

and

clang under Linux Mint 17.1 http://kpaste.net/836bc5f

While the warnings of gcc5 seem to be related only to implicit conversion in qDebug output, I think that the clang log is highlighting some possible bugs.

Ciao,
ABL
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Jojo-Schmitz
Hmm,
warning: format '%hhd' expects argument of type 'int'
This is nonsense, %hhd should not expect an int (but a signed char), that's
the whole point about it. %d expects an int

-----Original Message-----
From: ABL [mailto:[hidden email]]
Sent: Wednesday, May 06, 2015 6:28 PM
To: [hidden email]
Subject: Re: [Mscore-developer] review of compiler warnings

Since we are set to check all compiler warnings, here are the warnings that
still appear for:

gcc5 under Linux Mint 17.1 http://kpaste.net/9d9

and

clang under Linux Mint 17.1 http://kpaste.net/836bc5f

While the warnings of gcc5 seem to be related only to implicit conversion in
qDebug output, I think that the clang log is highlighting some possible
bugs.

Ciao,
ABL



--
View this message in context:
http://dev-list.musescore.org/review-of-compiler-warnings-tp7579292p7579305.
html
Sent from the MuseScore Developer mailing list archive at Nabble.com.

----------------------------------------------------------------------------
--
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications Performance
metrics, stats and reports that give you Actionable Insights Deep dive
visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Jojo-Schmitz
In reply to this post by ABL
And actually they had been inserted to avoid warnings in Mac

-----Original Message-----
From: Joachim Schmitz [mailto:[hidden email]]
Sent: Wednesday, May 06, 2015 8:02 PM
To: '[hidden email]'
Subject: RE: [Mscore-developer] review of compiler warnings

Hmm,
warning: format '%hhd' expects argument of type 'int'
This is nonsense, %hhd should not expect an int (but a signed char), that's
the whole point about it. %d expects an int

-----Original Message-----
From: ABL [mailto:[hidden email]]
Sent: Wednesday, May 06, 2015 6:28 PM
To: [hidden email]
Subject: Re: [Mscore-developer] review of compiler warnings

Since we are set to check all compiler warnings, here are the warnings that
still appear for:

gcc5 under Linux Mint 17.1 http://kpaste.net/9d9

and

clang under Linux Mint 17.1 http://kpaste.net/836bc5f

While the warnings of gcc5 seem to be related only to implicit conversion in
qDebug output, I think that the clang log is highlighting some possible
bugs.

Ciao,
ABL



--
View this message in context:
http://dev-list.musescore.org/review-of-compiler-warnings-tp7579292p7579305.
html
Sent from the MuseScore Developer mailing list archive at Nabble.com.

----------------------------------------------------------------------------
--
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications Performance
metrics, stats and reports that give you Actionable Insights Deep dive
visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

Jim Newton
In reply to this post by Jim Newton
Another small set of compiler warnings I get is from seq.cpp, lines 933. and 937.
/Users/jimka/MuseScore/mscore/seq.cpp:933:27: Absolute value function 'fabsf' given an argument of type 'qreal' (aka 'double') but has parameter of type 'float' which may cause truncation of value
/Users/jimka/MuseScore/mscore/seq.cpp:937:27: Absolute value function 'fabsf' given an argument of type 'qreal' (aka 'double') but has parameter of type 'float' which may cause truncation of value

The suggestion is to use std:abs instead of fabsf.

      for (unsigned i = 0; i < n; ++i) {
            qreal val = *p;
            lv = qMax(lv, fabsf(val));
            *p++ = val;

            val = *p;
            rv = qMax(lv, fabsf(val));
            *p++ = val;
            }
Reply | Threaded
Open this post in threaded view
|

Re: review of compiler warnings

lasconic
Administrator
These warnings are new with XCode 6.3.1 indeed. We could use std::abs() but I think we had a problem with some compilers/standard library not being happy with that. We could try though.

lasconic

2015-05-08 21:54 GMT+02:00 Jim Newton <[hidden email]>:
Another small set of compiler warnings I get is from seq.cpp, lines 933. and
937.
/Users/jimka/MuseScore/mscore/seq.cpp:933:27: Absolute value function
'fabsf' given an argument of type 'qreal' (aka 'double') but has parameter
of type 'float' which may cause truncation of value
/Users/jimka/MuseScore/mscore/seq.cpp:937:27: Absolute value function
'fabsf' given an argument of type 'qreal' (aka 'double') but has parameter
of type 'float' which may cause truncation of value

The suggestion is to use std:abs instead of fabsf.

      for (unsigned i = 0; i < n; ++i) {
            qreal val = *p;
            lv = qMax(lv, fabsf(val));
            *p++ = val;

            val = *p;
            rv = qMax(lv, fabsf(val));
            *p++ = val;
            }



--
View this message in context: http://dev-list.musescore.org/review-of-compiler-warnings-tp7579292p7579326.html
Sent from the MuseScore Developer mailing list archive at Nabble.com.

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
Mscore-developer mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/mscore-developer