Issues building on Windows with MinGW-w64

[brechtsanders]

As mentioned in this post on stackoverflow.com there is an issue building DuckDB with MinGW-w64 on Windows.
There is an error error: ISO C++ forbids comparison between pointer and integer [-fpermissive]
on this line:

auto compareNotEqual( LhsT const& lhs, RhsT&& rhs ) -> bool { return static_cast<bool>(lhs != rhs); }'

I have also find another issue due to the fact that DELETE is defined src/include/common/enums/statement_type.hpp while this is already defined somewhere else for Windows.

I can build successfully after applying these patches:

patch -ulbf third_party/catch/catch.hpp << EOF
@@ -1445 +1445 @@
-    auto compareNotEqual( LhsT const& lhs, RhsT&& rhs ) -> bool { return static_cast<bool>(lhs != rhs); }
+    auto compareNotEqual( LhsT const& lhs, RhsT&& rhs ) -> bool { return static_cast<bool>((uintptr_t)lhs != (uintptr_t)rhs); }
EOF
patch -ulbf src/include/common/enums/statement_type.hpp << EOF
@@ -22,2 +22,3 @@
        UPDATE,       // update statement type
+#undef DELETE
        DELETE,       // delete statement type
EOF

Though for the DELETE issue, it will be better if you could change it to something differeny.

[Mytherin]

Thanks for opening the issue! We would like to fix the issue, however, it seems like your patch works around the issues rather than addressing them. Thus while they do fix compilation, they do not fix the core problems that lead to the compilation problems in the first place.

For the first error error: ISO C++ forbids comparison between pointer and integer [-fpermissive] the issue does not lie with the catch framework, but rather with how it is used here in test_capi.cpp:

REQUIRE(stmt != NULL)

The correct fix here would be to change NULL to nullptr to prevent this error. I have pushed a commit that should fix it here: a565715.

The second issue seems to be an issue with a conflicting #define statement. This should also be solved in a different manner, but first we should figure out where the conflicting define comes from.

[brechtsanders]

In MinGW-w64 DELETE is defined in winnt.h as #define DELETE (__MSABI_LONG(0x00010000))
As it is used in the Win32 API you should stay away from it and use something else.

[Mytherin]

Ideally we would not include windows.h anywhere except where it is absolutely required (e.g. file_system.cpp) because of precisely such issues. Unfortunately catch seems to include it. I suppose this is one of the (major) disadvantages of header only libraries, any dependencies that the library has are dragged into everything.

I would suggest either (1) we undef common names (ERROR and DELETE) right after the windows.h include in catch.hpp, or (2) perhaps we could look into why catch is including windows.h at all as well, and move those parts outside of the header and into a separate file (catch.cpp).

[brechtsanders]

You can never be sure your library isn't included by code that also needs to include windows.h.
Instead of #undef you could also #define them into somethin else, but still this assumes these values will not be used for their Windows purpose in code that includes both this header and windows.h.
The only clean solution is to avoid these words, for example by adding a prefix as most libraries do.
Though I understand if your library already has some history and acceptance that may be the hardest solution.

[Mytherin]

Excellent point, indeed we should make duckdb.hpp compatible with windows.h. The problem right now is that the catch.hpp only includes windows.h if another (more slimmed down) header is not available, which is not the case for MSVC but is the case for MINGW. We should fix the issue and add a test that includes windows.h to prevent further clashes here. I will take a look at doing this soon.

[hannes]

We actually fixed this in the meantime.