[CLN-list] CLN and GiNaC patches for Win64 build

[Bruno Haible]

Jan Rheinländer wrote:
> I reviewed Robert's patches and added some of my own, that were
> necessary for GiNaC to work with CLN on Win64. They can be viewed on Github:
> 
> === CLN ===
> 
> https://github.com/jrheinlaender/cln/commits/win64
> 
> There are two kinds of patches:
> 
> - Patches required for Win64 platform (e.g. UL is 32 bit type on that
> platform). Mostly due to Robert (Number 1-10)
> 
> - Cosmetic patches to avoid unnecessary compiler warnings (e.g.
> confusion of class and struct)
> 
> Most patches I surrounded with #ifdef _M_AMD64 so that other platforms
> will be unaffected

Patch 01/10 looks good to me. All relevant platforms have <stdint.h> nowadays.
The last one to add it was MSVC, in the MSVC 2015 release. The last platform
that does NOT have <stdint.h> is IRIX 6.5, which is not a porting target any
more.

Patch 02/10 looks only partially right.
The #include <stdint.h> and the changes to sintP, uintP, sintV, uintV should
become unconditional.
On the other hand, the changes to sint32, uint32, sint64, uint64 look either
broken or - in the best case - worthless and confusing.

Patch 03/10 is only partially right:
* Include <stdint.h> always. Including it on a particular platform only will
  lead to maintenance problems.
* It is wrong to test _WIN64 like this. The macro _WIN64 is defined
    - in 64-bit mingw,
    - in 64-bit MSVC,
    - in 64-bit Cygwin *if and only if* some Windows API header gets included.
      See the file /usr/include/w32api/_cygwin.h on a 64-bit Cygwin:

      /* _WIN64 is defined by the compiler specs when targeting Windows.
         The Cygwin-targeting gcc does not define it by default, same as
         with _WIN32.  Therefore we set it here.  The result is that _WIN64
         is only defined if Windows headers are included. */
      #ifdef __x86_64__
      #define _WIN64
      #endif

  The correct test for the 64-bit native Windows platform is therefore
    defined(_WIN64) && !defined(__CYGWIN__)

* In the definitions of cl_tag_mask and cl_value_mask, it is simpler to write
    (uintptr_t)1
  instead of
    (uintptr_t)1UL

* The cl_combine overloads should be adjusted to be consistent with patch 02/10.

Patch 04/10 looks good.

Patch 05/10 is essentially good.
Here too, include <stdint.h> always.

Patch 06/10:
Please add an include <stdint.h> here too. Otherwise there is a risk that we
use the undefined type 'intptr_t'.

Patch 07/10:
* In cl_macros.h:
  Please simplify this: Instead of adding a case for '#if defined(_M_AMD64)',
  just change the !HAVE_FAST_LONGLONG case, replacing
  1L by (intptr_t)1
  2L by (intptr_t)2
  -1L by -(intptr_t)1
  -2L by -(intptr_t)2
* In cl_DS_mul_fftp.h there is no need to use intptr_t. The 'long' type was
  used here only to make sure we work on 32-bit words, as opposed to 16-bit
  words (in the Windows 3.1 port). For 15 years, we can assume 32-bit 'int'
  types already. Therefore just omit the 'L' suffix here.

Patch 08/10:
This looks wrong. I don't think you should cast an index of type 'size_t', ever.
Can you explain the problem/issue?

Patch 09/10:
This patch should be dropped, once you have stopped fiddling with
sint32, uint32, sint64, uint64 in patch 02/10.

Patch 10/10:
Include <stdint.h> always, here too.
Other that that, this patch looks OK.

Patch "Add missing delete operators to avoid compiler warning"
* The 'throw' declarations are correct.
* void operator delete (void* ptr, void* _ptr) { (void)ptr; (void)_ptr; }
  1) This exists only since C++14. So it is a portability problem to use it.
  2) Isn't it a memory leak if you define these methods to be a no-op?
  Reference: http://en.cppreference.com/w/cpp/memory/new/operator_delete

Patch "Remove user-definable malloc_hook and free_hook"
No no, you can't just remove a documented API like that.
What is the problem ("static initialization fiasco")? It must have a better
solution.

Patch "Fix bugs that result from the fact that type UL is 16bit on Win64"
* Please don't write "Win64" platforms. That sounds like it would be a "win"
  to use such a platform. Better write "64-bit Windows" instead.
* 'unsigned long' is 32-bit, not 16-bit, no?
* The cl_low.h change looks correct.
* In cl_I_doublefactorial.cc and cl_I_factorial.cc please don't duplicate code.
  How about writing (uintptr_t)xxx instead of xxxUL. Will that fix the issue?

Patch "Force correct underlying type of float_format_t for Win64 platform"
* What is the issue/symptom, and why does it not appear with other compilers
  than MSVC?

Patch "Fix an inconsistency (class used in definition where struct ..."
Looks good. In the old times, CLN required g++, and for g++ 'friend class'
and 'friend struct' are/were synonymous.

Bruno