Hello Bruno, thanks for your detailed comments. I do not have the "big picture" of all the supported platforms so I put in a lot of conditional code to be on the safe side. I will re-work the patches as you suggest. About the "static initialization order fiasco": I learned about this by searching the web. It seems that the C++ standard does not guarantee any kind of order for the initialization of static variables. Therefore, it is unknown when exactly free_hook and malloc_hook pointers will be assigned a value. I verified it in the Visual Studio debugger. integral::relative_integration_error is is an expression that is initialized as 1e-8 which results in constructing a cl_DF. This requires malloc_hook and free_hook to be initialized, but in my case only malloc_hook was, whereas free_hook was 0x0. free_hook is invoked here (the temporary cl_DF is deleted when the method returns). inline const cl_F cl_float (double x, float_format_t y) { return cl_float(cl_DF(x),y); } I don't see any way around this problem other than removing the user-definable hooks (certainly nobody is using those hooks on Windows x64 platform currently ...) About the "Force correct underlying type of float_format_t for Win64 platform": I posted about this previously (12.3.2018). The problem certainly exists with VS 2015 compiler. Jan Am 01.05.2018 um 19:15 schrieb Bruno Haible:
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
Jan Rheinländer wrote: 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