1.4 build failing on OSX for me

[Ian MacArthur]

Just did a git pull and a make (camp build) and it failed with this:


[ 43%] Building CXX object src/CMakeFiles/fltk.dir/drivers/PostScript/Fl_PostScript_image.cxx.o
/Users/ian/src/Git/fltk-1.4/src/drivers/PostScript/Fl_PostScript_image.cxx:47:26: error: call to 'abs' is ambiguous
memcpy(buf, curdata, abs(cb_data->D));
^~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:733:1: note:
candidate function
abs(float __lcpp_x) _NOEXCEPT {return ::fabsf(__lcpp_x);}
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:737:1: note:
candidate function
abs(double __lcpp_x) _NOEXCEPT {return ::fabs(__lcpp_x);}
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h:741:1: note:
candidate function
abs(long double __lcpp_x) _NOEXCEPT {return ::fabsl(__lcpp_x);}
^


Never seen that before... This is on..

$ uname -a
Darwin immpcX.local 17.7.0 Darwin Kernel Version 17.7.0: Fri Oct 30 13:34:27 PDT 2020; root:xnu-4570.71.82.8~1/RELEASE_X86_64 x86_64


ian$ gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
Apple LLVM version 10.0.0 (clang-1000.11.45.5)
Target: x86_64-apple-darwin17.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

[Ian MacArthur]

> On 21 Mar 2021, at 21:49, Ian MacArthur wrote:
>
> Just did a git pull and a make (camp build) and it failed with this:

My spelling is atrocious... “camp” build? I was going for cmake...

Anyway, the filling line is one of many, and has not changed since last October, so it is not this line that is *actually* different.

Rather, I think something is different about the include files now.
It might be a side-effect of Albrecht’s change?

commit 41266df7ae51f179f4a80f67a2eba896ba1fd7b2
Author: Albrecht Schlosser <albrech...@online.de>
Date: Sat Mar 20 21:39:28 2021 +0100

Remove unnecessary system includes from public headers

Add includes of system headers in the implementation files
where necessary.

[Ian MacArthur]

On 21 Mar 2021, at 22:00, Ian MacArthur wrote:
> On 21 Mar 2021, at 21:49, Ian MacArthur wrote:
>>
>> Just did a git pull and a make (camp build) and it failed with this:
>
>
> My spelling is atrocious... “camp” build? I was going for cmake...
>
> Anyway, the filling line is one of many, and has not changed since last October, so it is not this line that is *actually* different.

And that is meant to be “failing” nor “filling"...

[Albrecht Schlosser]

On 3/21/21 11:00 PM Ian MacArthur wrote:
>
>> On 21 Mar 2021, at 21:49, Ian MacArthur wrote:
>>
>> Just did a git pull and a make (camp build) and it failed with this:
>
> My spelling is atrocious... “camp” build? I was going for cmake...

Works fine for me on my M1 mac, both configure/make and cmake.

$ uname -a
Darwin mars 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:06:51 PST
2021; root:xnu-7195.81.3~1/RELEASE_ARM64_T8101 arm64

$ gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr
--with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1

Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: arm64-apple-darwin20.3.0

Thread model: posix
InstalledDir:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

> Anyway, the filling line is one of many, and has not changed since last October, so it is not this line that is *actually* different.
>
> Rather, I think something is different about the include files now.
> It might be a side-effect of Albrecht’s change?
>
> commit 41266df7ae51f179f4a80f67a2eba896ba1fd7b2
> Author: Albrecht Schlosser <albrech...@online.de>
> Date: Sat Mar 20 21:39:28 2021 +0100
>
> Remove unnecessary system includes from public headers
>
> Add includes of system headers in the implementation files
> where necessary.

Sure, this is possible, but I have no clue what might cause this
"ambiguity".

Two questions:

(1) did you clean up the build (if not can you try again)?

(2) can you roll back to the commit before mine and try with this one?

Otherwise, bisecting could help to find the exact commit that made it fail.

Or ... that's something for Manolo, maybe.

[Albrecht Schlosser]

FTR: Most of my changes concerned stdlib.h and stdio.h, but also some
others. I did not touch this file specifically, but changes in headers
could, of course, have affected this.

Other than that, there have been recent changes in this file in Feb. and
March:

$ git lp -3 -- src/drivers/PostScript/Fl_PostScript_image.cxx
1fbcae13b 2021-03-15 14:09:50 +0100 ManoloFLTK - Create class
Fl_Cairo_Graphics_Driver.
a3cb4af73 2021-03-15 08:13:21 +0100 ManoloFLTK - Class
Fl_PostScript_Graphics_Driver: better separation of what varies with
USE_PANGO
9fad60140 2021-02-13 21:13:04 +0100 ManoloFLTK - Remove
compilation warnings issued by Visual Studio 2019.

[Ian MacArthur]

On 21 Mar 2021, at 22:09, Albrecht Schlosser wrote:
>
> $ uname -a
> Darwin mars 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:06:51 PST 2021; root:xnu-7195.81.3~1/RELEASE_ARM64_T8101 arm64
> $ gcc -v
> Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/4.2.1
> Apple clang version 12.0.0 (clang-1200.0.32.29)
> Target: arm64-apple-darwin20.3.0
> Thread model: posix
> InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

That is a fair bit newer than I have - may Mac is “old” now... still 10.13.6 which is the latest available for it...

> Sure, this is possible, but I have no clue what might cause this "ambiguity”.

Me neither...

> Two questions:
>
> (1) did you clean up the build (if not can you try again)?

Yes.
Both cmake and autoconf builds fail, as it turns out.

>
> (2) can you roll back to the commit before mine and try with this one?

git checkout bd52db0b952d35ce793153e4a0c0b25f102a4395
make clean ; make -j6

All works fine

That checkout is...
Author: Greg Ercolano
Date: Fri Mar 19 09:22:01 2021 -0700

Added docs for public fl_vsnprintf() (STR #3413)


(works fine)


git checkout 41266df7ae51f179f4a80f67a2eba896ba1fd7b2
make clean ; make -j6

Fails as reported.

So there’s something weird with the include files hierarchy now, but I have literally no idea what is going on.

[Ian MacArthur]

On 21 Mar 2021, at 22:28, Ian MacArthur wrote:
>
> So there’s something weird with the include files hierarchy now, but I have literally no idea what is going on.

Though I do note that the failing lines are because the compiler is trying to decide which version of fabs to call (fabs, fabsf, fabsl) and can’t because the data being processed is actually an int.

e.g.

buf += abs(cb_data->D);

where D is an int type.


So... I’m not sure show that used to work, since I cannot see where we have declared an integer abs() function in this code...

[Albrecht Schlosser]

Well, I think I found the cause - but not exactly the reason why it's
different on your system and mine.

- all "alternatives" shown in your error message have float or double
arguments

- cb_data->D is 'int'

- abs(int) is declared in stdlib.h (sic!)

This means that my changes *did* affect this issue.

But then I added this code to the file in question after all other headers:

#ifndef _STDLIB_H_
#warning "stdlib.h" not included - including now ..."
#include <stdlib.h>
#endif

... and got NO WARNING. I could also see in my editor (VS Code) that
stdlib.h was indeed included.

So the question remains why this is not the case in your build. Maybe
different configurations/options?

Anyway, can you please add this code (above) to your file to verify that
you get the warning? Compilation should succeed then.

That said, since the source code uses abs(int) we definitely need
stdlib.h and I'm going to add it.

Would be interesting to see your test results though and what
configuration you used. TIA.

[Albrecht Schlosser]

See my previous message: we need stdlib.h which declares abs(int).

[Ian MacArthur]

On 21 Mar 2021, at 22:46, Albrecht Schlosser wrote:
>
> But then I added this code to the file in question after all other headers:
>
> #ifndef _STDLIB_H_
> #warning "stdlib.h" not included - including now ..."
> #include <stdlib.h>
> #endif
>
> ... and got NO WARNING. I could also see in my editor (VS Code) that stdlib.h was indeed included.
>
> So the question remains why this is not the case in your build. Maybe different configurations/options?
>
> Anyway, can you please add this code (above) to your file to verify that you get the warning? Compilation should succeed then.
>
> That said, since the source code uses abs(int) we definitely need stdlib.h and I'm going to add it.
>
> Would be interesting to see your test results though and what configuration you used. TIA.

Yup, that’s the one...

ian$ make
=== making src ===
Compiling drivers/PostScript/Fl_PostScript_image.cxx...
drivers/PostScript/Fl_PostScript_image.cxx:36:2: warning: "stdlib.h" not included - including now ..." [-W#warnings]

#warning "stdlib.h" not included - including now ..."

^
1 warning generated.
/usr/bin/ar cr ../lib/libfltk.a ...
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../lib/libfltk.a(fl_overlay_visual.o) has no symbols



I’d only got as far as adding:

#define abs(XX) (((XX)>=0)?(XX):(-XX))

(which also worked fine)

Cheers, think that’s a solution then... You are adding stdlib.h to Fl_PostScript_image.cxx?

[Albrecht Schlosser]

Yes and yes.

Meanwhile I found out that my (macOS) version of 'math.h' includes
'stdlib.h' which explains why it is included (I analyzed the
preprocessor output, it's not just a guess!).

I don't know where stdlib.h got included on my Linux system (but I think
I'll check this as well).

From my Mac:

$ grep -n2 stdlib.h
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/math.h
299-
300-#define _LIBCPP_STDLIB_INCLUDE_NEXT
301:#include <stdlib.h>
302-
303-#include_next <math.h>
--
--
764-#endif
765-
766:// MSVCRT already has the correct prototype in <stdlib.h> if
__cplusplus is defined
767-#if !defined(_LIBCPP_MSVCRT) && !defined(__sun__) && !defined(_AIX)
768-inline _LIBCPP_INLINE_VISIBILITY long abs(long __x) _NOEXCEPT {
--
--
800-#endif
801-
802:// MSVCRT already has the correct prototype in <stdlib.h> if
__cplusplus is defined
803-#if !defined(_LIBCPP_MSVCRT) && !defined(__sun__) && !defined(_AIX)
804-inline _LIBCPP_INLINE_VISIBILITY ldiv_t div(long __x, long __y)
_NOEXCEPT {


Line #301 includes stdlib.h.

I have no idea what 'MSVCRT' has to do with macOS though. ;-)

[Albrecht Schlosser]

On 3/22/21 12:06 AM Albrecht Schlosser wrote:
> On 3/21/21 11:53 PM Ian MacArthur wrote:
>> On 21 Mar 2021, at 22:46, Albrecht Schlosser wrote:
>>
>> ... You are adding stdlib.h to Fl_PostScript_image.cxx?

Done. commit 62ff323b706b0ea24f079462de987cf73981dcbe

[Greg Ercolano]

On 3/21/21 3:53 PM, Ian MacArthur wrote:

On 21 Mar 2021, at 22:46, Albrecht Schlosser wrote:

[..]
Anyway, can you please add this code (above) to your file to verify that you get the warning? [..]

Yup, that’s the one...

    Glad it's solved..

    While we're looking at that draw_image_cb() code, since abs() is called twice
    for the same value inside what looks like a pixel loop, we could probably improve
    performance by setting the variable once outside the loop, e.g. (not tested!):

---

  cb_data = (struct callback_data*)data;
  const size_t abs_D = abs(cb_data->D); // SET ONCE OUTSIDE LOOP
  int last = x+w;
  curdata = cb_data->data + x*cb_data->D + y*cb_data->LD;
  for (; x<last; x++) {
    memcpy(buf, curdata, abs_D); // WAS: memcpy(buf, curdata, abs(cb_data->D));
    buf += abs_D;                // WAS: buf += abs(cb_data->D);
    curdata += cb_data->D;
  }

---

    If I read this code correctly, cb_data->D never changes within the loop,
    so there's no reason to keep recalculating it.

[Albrecht Schlosser]

On 3/22/21 12:24 AM Greg Ercolano wrote:
>
> On 3/21/21 3:53 PM, Ian MacArthur wrote:
>> On 21 Mar 2021, at 22:46, Albrecht Schlosser wrote:
>>> [..]
>>> Anyway, can you please add this code (above) to your file to verify that you get the warning? [..]
>> Yup, that’s the one...
>
>     Glad it's solved..

Yes, me too. That was a big surprise!

>     While we're looking at that draw_image_cb() code, since abs() is
> called twice
>     for the same value inside what looks like a pixel loop, we could
> probably improve
>     performance by setting the variable once outside the loop, e.g.
> (not tested!):
>

> ------------------------------------------------------------------------

>   cb_data = (struct callback_data*)data;
>   const size_t abs_D = abs(cb_data->D); // SET ONCE OUTSIDE LOOP
>   int last = x+w;
>   curdata = cb_data->data + x*cb_data->D + y*cb_data->LD;
>   for (; x<last; x++) {
>     memcpy(buf, curdata, abs_D); // WAS: memcpy(buf, curdata,
> abs(cb_data->D));
>     buf += abs_D;             // WAS: buf += abs(cb_data->D);
>     curdata += cb_data->D;
>   }

> ------------------------------------------------------------------------

>
>     If I read this code correctly, cb_data->D never changes within the
> loop,
>     so there's no reason to keep recalculating it.

;-) Are you sure the compiler wouldn't do this for us anyway? :-)

Honestly: +1

Looks plausible and clarifies the code and makes it easier to understand
((w/o the "WAS: comments", please)).

(Disclaimer: neither tested here.)

Feel free to do it, here it's too late for today anyway...