1.7.1 Compilation errors on Solaris with SunStudio C++ 12.4

[prrace]

There were several issues here and I made source code changes .. probably
all of these could be wrapped in #ifdef __SUNPRO_CC if so desired.

harfbuzz/hb-face.cc", line 493: Error: The operation "extern "C" void()(void) != void()(void)" is illegal.

The line it did not like is
if (face->destroy != _hb_face_for_data_closure_destroy)

Quite why that comparison is considered illegal is not apparent to me.
I don't know what the elegant solution is here. I simply hacked it to be
if ((void*)(face->destroy) != (void*)_hb_face_for_data_closure_destroy)

and the compiler was happy, but I don't think of this as a long term solution.

hb-dsalgs.hh", line 80: Error: Badly formed expression.
hb-dsalgs.hh", line 82: Error: Use ";" to terminate declarations.
hb-dsalgs.hh", line 82: Error: "," expected instead of ")".
hb-dsalgs.hh", line 82: Error: Use ";" to terminate declarations.
hb-dsalgs.hh", line 82: Error: A declaration was expected instead of ",".
hb-dsalgs.hh", line 83: Error: "," expected instead of ")".
hb-dsalgs.hh", line 85: Error: Use ";" to terminate declarations.
hb-dsalgs.hh", line 85: Error: A declaration was expected instead of ",".
hb-dsalgs.hh", line 85: Error: a is not defined.
hb-dsalgs.hh", line 85: Error: w is not defined.
hb-dsalgs.hh", line 86: Error: A declaration was expected instead of "if".
hb-dsalgs.hh", line 86: Error: a is not defined.

It turns out that the SunStudio compiler does not support "restrict" so this line was not parsed correctly :
static int sort_r_cmpswap(char *__restrict a, char *__restrict b, size_t w,

I changed it to
#ifndef __SUNPRO_CC
/* __restrict is same as restrict but better support on old machines */
static int sort_r_cmpswap(char *__restrict a, char *__restrict b, size_t w,
#else
static int sort_r_cmpswap(char *a, char *b, size_t w,
#endif

We lose the verification of __restrict but at least it compiles.

3. hb-string-array.hh", line 51: Error: An array cannot have zero size unless you use the option -features=zla.

That turns out to be the union member str in this code :
#undef _S
} st;
char str[0];

Since you need 'str' just to get the offset of the beginning of the array that is the other
member of this union and that is WAY bigger than the one extra byte I decided it was
easiest and harmless to just change it to

char str[1];

[jfkthame]

harfbuzz/hb-face.cc", line 493: Error: The operation "extern "C" void(/)(void/) != void(/)(void/)" is illegal. The line it did not like is if (face->destroy != _hb_face_for_data_closure_destroy) Quite why that comparison is considered illegal is not apparent to me. I don't know what the elegant solution is here. I simply hacked it to be if ((void*)(face->destroy) != (void*)_hb_face_for_data_closure_destroy) and the compiler was happy, but I don't think of this as a long term solution. Is it complaining because hb_destroy_func_t is declared (in hb-common.h) with extern "C" linkage, but _hb_face_for_data_closure_destroy is not; and so their types don't match? If so, does it help if you wrap the _hb_face_for_data_closure_destroy function in hb-face.cc with extern "C" {...} as well?

[prrace]

In fact wrapping that declaration in extern "C" Is actually what I have now and I should
have said so in the first place.
The bug submission text was a copy/paste from an earlier email that is superseded.

[behdad]

Is it complaining because hb_destroy_func_t is declared (in hb-common.h) with extern "C" linkage, but _hb_face_for_data_closure_destroy is not; and so their types don't match?

Right. But it's not clear why only this instance errs for Phil. (We sorted this out offline before; he's just reporting here now.)

[ebraminio]

We can not see these errors with ODS 12.6 https://circleci.com/gh/harfbuzz/harfbuzz/604

Is it because of version difference or the fact our CI uses CMake? Can you help us on reproducing these issues by proposing needed change on these lines https://github.com/harfbuzz/harfbuzz/blob/master/.circleci/config.yml#L5-L13 on our CI?

[prrace]

I am in the first steps of the process of updating harfbuzz in JDK 11 and I still see the
problem
harfbuzz/hb-face.cc", line 493: Error: The operation "extern "C" void(/)(void/) != void(/)(void/)" is illegal.

and I still need to work around it by wrapping the definition like so
#ifdef __SUNPRO_CC
extern "C" {
#endif
static void
_hb_face_for_data_closure_destroy (void *data)
{
hb_face_for_data_closure_t *closure = (hb_face_for_data_closure_t *) data;

hb_blob_destroy (closure->blob);
free (closure);
}
#ifdef __SUNPRO_CC
}
#endif

Note that JDK still uses SS12.4 so it is possible that if you don't see it with ODS 12.6
that this has "fixed" it. Or it could be that the Linux ODS you are using is using
some different code ..

However I also have some new errors on Solaris that came in with this commit.
82eb1da

There are 3 of them and they all look like this
make[3]: *** [/build/solaris-sparcv9/support/native/java.desktop/libfontmanager/hb-face.o] Error 1
make[3]: *** Waiting for unfinished jobs....
"/open/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-private.hh", line 435: Error: The last statement should return a value.
"/open/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-open-type-private.hh", line 1116: Where, temwhileinst: While instantiating "_hb_bit_storage(unsigned)".
"/open/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-open-type-private.hh", line 1116: Where, teminstend: Instantiated from non-template code.
"/open/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-private.hh", line 362: Error: The last statement should return a value.
"/open/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-map.cc", line 142: Where, temwhileinst: While instantiating "_hb_popcount<hb_glyph_flags_t>(hb_glyph_flags_t)".
"/open/src/java.desktop/share/native/libfontmanager/harfbuzz/hb-ot-map.cc", line 142: Where, teminstend:
~
and they are due to the three cases of "assert (0)" being the last line of a function
that is supposed to return an int in hb-private.hh
One solution, which is what I am using, is adding "return (0);" after the assert.

[behdad]

However I also have some new errors on Solaris that came in with this commit.
82eb1da

Fixed these.

[behdad]

Phil,

Does a cast help:

diff --git a/src/hb-face.cc b/src/hb-face.cc
index b8d7e84d..2fef09d0 100644
--- a/src/hb-face.cc
+++ b/src/hb-face.cc
@@ -514,7 +514,7 @@ hb_face_get_table_tags (hb_face_t    *face,
                        unsigned int *table_count, /* IN/OUT */
                        hb_tag_t     *table_tags /* OUT */)
 {
-  if (face->destroy != _hb_face_for_data_closure_destroy)
+  if (face->destroy != (hb_destroy_func_t) _hb_face_for_data_closure_destroy)
   {
     if (table_count)
       *table_count = 0;

[behdad]

Is it complaining because hb_destroy_func_t is declared (in hb-common.h) with extern "C" linkage, but _hb_face_for_data_closure_destroy is not; and so their types don't match?

Right. But it's not clear why only this instance errs for Phil. (We sorted this out offline before; he's just reporting here now.)

Ok, I see why only this errs now. It's the comparison that is failing. I suppose when trying to use _hb_face_for_data_closure_destroy as a destroy function the compiler does whatever magic cast to make it happen? So, I think an explicit cast might fix the comparison.

[ebraminio]

As #1053