Changes to stable-5-0 branch

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

Changes to stable-5-0 branch

Bastian Märkisch
Hi,

Today I checked in some changes to the stable-5-0 branch which make
gnuplot sources compatible with MSYS2/Mingw-w64 on Windows, as well as
they make wxWidgets 3.0 work.  These changes should in principle be
compatible with other platforms, but I could only check on CentOS5 & 6.
Of course it still compiles cleanly using MSVC2012 or the old MSYS/MinGW
combination, too.

There's another patch I would like to include, which avoids warnings
about incompatible linkage of wxEvents on Windows.  It works on CentOS6,
too.  Could someone please verify it does not break e.g. the Mac build?

   Bastian

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta

wxt_local_event.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Changes to stable-5-0 branch

sfeam
On Saturday, 13 February 2016 06:34:27 PM Bastian Märkisch wrote:
> Hi,
>
> Today I checked in some changes to the stable-5-0 branch which make
> gnuplot sources compatible with MSYS2/Mingw-w64 on Windows, as well as
> they make wxWidgets 3.0 work.  These changes should in principle be
> compatible with other platforms, but I could only check on CentOS5 & 6.
> Of course it still compiles cleanly using MSVC2012 or the old MSYS/MinGW
> combination, too.

In general it is not good to make changes in the stable branch that have not
already been tested in the main branch (5.1).  Obviously there are exceptions
like fixing a bug that is only present in the stable branch.

In particular the change below is problematic because it was already tried in
the main branch and turned out to cause problems on some systems:

        * src/qtterminal/qt_term.h src/qtterminal/qt_conversion.cpp:  isnan()
        is in namespace std.

See 5.1 ChangeLog:

2015-12-10  Hans-Bernhard Broeker  <[hidden email]>

        * src/qtterminal/qt_conversion.cpp (qt_imageToQImage): Do not call
        C++ isnan() without a namespace specifier.

        EAM:  Reverting this change.  We may need a fix, but this isn't it.
        qtterminal/qt_conversion.cpp:
        In function 'QImage qt_imageToQImage(int, int, coordval*, t_imagecolor)':
        qtterminal/qt_conversion.cpp:129:14:
        error: expected unqualified-id before '(' token if (std::isnan(*image))

I don't know what the issue is here, but since adding the std:: qualifier
is known to break the build on some systems that were perfectly happy before
this, I think it is not suitable for the stable branch.  

> There's another patch I would like to include, which avoids warnings
> about incompatible linkage of wxEvents on Windows.  It works on CentOS6,
> too.  Could someone please verify it does not break e.g. the Mac build?

It works for me on linux (tested on 2 systems), but again I'd be happier if it went
into 5.1 before applying it to the stable branch.

        Ethan

>    Bastian


------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta
Reply | Threaded
Open this post in threaded view
|

Re: Changes to stable-5-0 branch

tmacchant
----- Original Message -----

> From: sfeam 
> To: gnuplot-beta
> Cc: Bastian Märkisch 
> Date: 2016/2/14, Sun 05:23
> Subject: Re: Changes to stable-5-0 branch
>
> On Saturday, 13 February 2016 06:34:27 PM Bastian Märkisch wrote:
>>  Hi,
>>
>>  Today I checked in some changes to the stable-5-0 branch which make
>>  gnuplot sources compatible with MSYS2/Mingw-w64 on Windows, as well as
>>  they make wxWidgets 3.0 work.  These changes should in principle be
>>  compatible with other platforms, but I could only check on CentOS5 & 6.
>>  Of course it still compiles cleanly using MSVC2012 or the old MSYS/MinGW
>>  combination, too.
>
> In general it is not good to make changes in the stable branch that have not
> already been tested in the main branch (5.1).  Obviously there are exceptions
> like fixing a bug that is only present in the stable branch.
>
> In particular the change below is problematic because it was already tried in
> the main branch and turned out to cause problems on some systems:
>
>         * src/qtterminal/qt_term.h src/qtterminal/qt_conversion.cpp:  isnan()
>         is in namespace std.

I have been used this isnan change for MinGW-w64 (both 32 and 64bit)
without any reports.

Tatsuro

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta
Reply | Threaded
Open this post in threaded view
|

Re: Changes to stable-5-0 branch

Bastian Märkisch
In reply to this post by sfeam


Am 13.02.2016 um 21:23 schrieb sfeam:
(snip)

> In particular the change below is problematic because it was already tried in
> the main branch and turned out to cause problems on some systems:
>
>          * src/qtterminal/qt_term.h src/qtterminal/qt_conversion.cpp:  isnan()
>          is in namespace std.
>
> See 5.1 ChangeLog:
>
> 2015-12-10  Hans-Bernhard Broeker  <[hidden email]>
>
>          * src/qtterminal/qt_conversion.cpp (qt_imageToQImage): Do not call
>          C++ isnan() without a namespace specifier.
>
>          EAM:  Reverting this change.  We may need a fix, but this isn't it.
>          qtterminal/qt_conversion.cpp:
>          In function 'QImage qt_imageToQImage(int, int, coordval*, t_imagecolor)':
>          qtterminal/qt_conversion.cpp:129:14:
>          error: expected unqualified-id before '(' token if (std::isnan(*image))
>
> I don't know what the issue is here, but since adding the std:: qualifier
> is known to break the build on some systems that were perfectly happy before
> this, I think it is not suitable for the stable branch.
>

OK. Reverted in branch-5-0-stable.

The problem is that isnan is in the C99 standard, but wasn't in the C++
standard until C++11. Hence, many C++ compilers implemented it "their"
way, as _isnan (MSVC), ::isnan, std::isnan or macro (as in C99). See
e.g. the discussion at
http://stackoverflow.com/questions/570669/checking-if-a-double-or-float-is-nan-in-c
Btw. simply adding a "using namespace std" doesn't solve the issue
either, since some compilers have both, ::isnan and std::isnan.

Too me it looks like the only way of solving this in a portable way is
to implement a C function like this in e.g. stdfn.c|h :

TBOOLEAN gp_isnan(double v)
{
        return isnan(v);
}


   Bastian

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta
Reply | Threaded
Open this post in threaded view
|

Re: Changes to stable-5-0 branch

sfeam
On Sunday, 14 February 2016 10:46:24 AM Bastian Märkisch wrote:

>
> Am 13.02.2016 um 21:23 schrieb sfeam:
> (snip)
> > In particular the change below is problematic because it was already tried in
> > the main branch and turned out to cause problems on some systems:
> >
> >          * src/qtterminal/qt_term.h src/qtterminal/qt_conversion.cpp:  isnan()
> >          is in namespace std.
> >
> > See 5.1 ChangeLog:
> >
> > 2015-12-10  Hans-Bernhard Broeker  <[hidden email]>
> >
> >          * src/qtterminal/qt_conversion.cpp (qt_imageToQImage): Do not call
> >          C++ isnan() without a namespace specifier.
> >
> >          EAM:  Reverting this change.  We may need a fix, but this isn't it.
> >          qtterminal/qt_conversion.cpp:
> >          In function 'QImage qt_imageToQImage(int, int, coordval*, t_imagecolor)':
> >          qtterminal/qt_conversion.cpp:129:14:
> >          error: expected unqualified-id before '(' token if (std::isnan(*image))
> >
> > I don't know what the issue is here, but since adding the std:: qualifier
> > is known to break the build on some systems that were perfectly happy before
> > this, I think it is not suitable for the stable branch.
> >
>
> OK. Reverted in branch-5-0-stable.
>
> The problem is that isnan is in the C99 standard, but wasn't in the C++
> standard until C++11. Hence, many C++ compilers implemented it "their"
> way, as _isnan (MSVC), ::isnan, std::isnan or macro (as in C99). See
> e.g. the discussion at
> http://stackoverflow.com/questions/570669/checking-if-a-double-or-float-is-nan-in-c
> Btw. simply adding a "using namespace std" doesn't solve the issue
> either, since some compilers have both, ::isnan and std::isnan.

I am having trouble to understand how in our particular case C++ can
fail to find the C language definition of isnan() from <math.h>

The source line at issue is

qt_conversion.cpp:129:                          if (isnan(*image))
included from
qt_term.cpp:78:#include "qt_conversion.cpp"

But before this we have
qt_term.cpp:57: #include "term_api.h"  // for stdfn.h, JUSTIFY, encoding, ...
term_api.h:43:#include "stdfn.h"
stdfn.h:291:# include <math.h>

and <math.h> as I understand it is required to define isnan() by C99.
So why does the C++ compiler not pick up the C99 macro even if
it wouldn't otherwise define it on its own?

>
> Too me it looks like the only way of solving this in a portable way is
> to implement a C function like this in e.g. stdfn.c|h :
>
> TBOOLEAN gp_isnan(double v)
> {
> return isnan(v);
> }

If it's only this one place, may the simplest work-around is:

--- old/qt_conversion.cpp  2016-02-14 13:51:54.866487634 -0800
+++ new/qt_conversion.cpp     2016-02-15 14:29:15.785792815 -0800
@@ -126,7 +126,7 @@ QImage qt_imageToQImage(int M, int N, co
                        QRgb* line = (QRgb*)(qimage.scanLine(n));
                        for (int m = 0; m < M; m++)
                        {
-                               if (isnan(*image))
+                               if (*image != *image)   // this test works even if isnan() is missing
                                {
                                        image++;
                                        *line++ = 0x00000000;



        Ethan

>
>    Bastian



------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta
Reply | Threaded
Open this post in threaded view
|

Re: Changes to stable-5-0 branch

Hans-Bernhard Bröker-2
Am 15.02.2016 um 23:31 schrieb sfeam:

> I am having trouble to understand how in our particular case C++ can
> fail to find the C language definition of isnan() from <math.h>
>
> and <math.h> as I understand it is required to define isnan() by C99.

It would be.  _But_ we're not running a C99 compiler here; we're running
a C++ compiler.  Now a C++11 compiler would be required (C++1x
26.8p3,p4) to effectively pull in the C99 version of <math.h>, including
isnan(), except that in C++ it's not a macro: it's a set of three
overloaded functions.  C++ really doesn't want macros for this sort of
thing.

A compiler that's not running in C++11 mode (or equivalent) is not only
not required to do that: it's even kind-of forbidden.  Earlier editions
of the C++ standard explicitly listed which functions <math.h> shall
bring into a C++ program's global name space (C++03 26.5p1,p2). isnan()
was not among them.  To overcome this limit might take an equivalent of
what the AC_USE_SYSTEM_EXTENSIONS method of autoconf does for us about
the difference between "strict" and "extended" C, but for C++.

> So why does the C++ compiler not pick up the C99 macro even if
> it wouldn't otherwise define it on its own?

Because the header itself hides it from C++'s view, probably by querying
(__STDC_VERSION__ >= 199901L) and/or (__cplusplus >= 201103L)

> If it's only this one place, may the simplest work-around is:
> -                               if (isnan(*image))
> +                               if (*image != *image)   // this test works even if isnan() is missing

That might well be the case.


------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
gnuplot-beta mailing list
[hidden email]
Membership management via: https://lists.sourceforge.net/lists/listinfo/gnuplot-beta