Skip to content

Conversation

isaacbrodsky
Copy link
Collaborator

#366

This PR is still a draft. It changes how DLLs are built on Windows.

  • Changed from exporting all symbols to export only the public API. This gets filters and generators to work but the benchmarks (due to benchmarkPolygon) and tests (many tests use internal functions) do not work for shared libs.
  • Generators needed a data table copied over rather than referencing it.
  • Filter applications needed a small change to how they detect pentagons when iterating over the H3 grid.
  • BUILD_ALLOC_TESTS option added to disable the custom allocator tests. It's not clear to me if the way DLLs work on Windows will support the unresolved symbol mechanism currently being offered for *nix systems. If it doesn't seem possible to support, I'd suggest disabling that feature on Windows. If needed, there could be a separate option where malloc/etc can be passed as function pointers, similar to how SDL works. This would be an option, so platforms that don't need this won't have the indirection overhead.

TODO:

  • Add shared lib tests in CI
  • Determine what to do about the tests for shared libs on Windows.
  • Possibly disable the alloc tests on Windows


## Building shared libraries

You can build H3 as a shared library (DLL), but the test suite does not support this configuration because the tests use functions internal to the DLL, and they are not exposed for testing.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can drop everything after the comma.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last comma, I mean.

@@ -29,10 +29,19 @@
#ifdef H3_ALLOC_PREFIX
#define H3_MEMORY(name) TJOIN(H3_ALLOC_PREFIX, name)

#ifdef __cplusplus
extern "C" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is Microsoft going to give up this stupid fight and compile normal C?

@@ -152,46 +163,47 @@ typedef struct {
*/
/** @brief find the H3 index of the resolution res cell containing the lat/lng
*/
H3Index H3_EXPORT(geoToH3)(const GeoCoord *g, int res);
DECLSPEC H3Index H3_EXPORT(geoToH3)(const GeoCoord *g, int res);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not going to block on this, but I wonder if it would have been possible to modify the H3_EXPORT instead to tackle this, eg: H3_EXPORT(H3Index, geoToH3)(const GeoCoord *g, int res); and macro args may or may not include the DECLSPEC thing.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.188% when pulling 537ad73 on isaacbrodsky:windows-tests into ada0062 on uber:master.

@isaacbrodsky isaacbrodsky marked this pull request as ready for review October 18, 2020 22:05
@isaacbrodsky isaacbrodsky merged commit 6c3c806 into uber:master Nov 9, 2020
@isaacbrodsky isaacbrodsky deleted the windows-tests branch November 9, 2020 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants