Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export classes and functions from library for Windows & Linux #244

Closed
damiandixon opened this issue Sep 30, 2022 · 19 comments
Closed

Export classes and functions from library for Windows & Linux #244

damiandixon opened this issue Sep 30, 2022 · 19 comments

Comments

@damiandixon
Copy link
Contributor

The DLL exporting of classes and methods on Windows requires them to be exported using:

#ifndef CLIPPER2_DLL
#  if defined(_WIN32)
#    ifdef CLIPPER2_DLL_EXPORT
#      define CLIPPER2_DLL __declspec(dllexport)
#    else
#      define CLIPPER2_DLL __declspec(dllimport)
#    endif
#  else
#    if __GNUC__ >= 4
#      define CLIPPER2_DLL __attribute__((visibility("default")))
#    else
#      define CLIPPER2_DLL
#    endif
#  endif
#endif

The above should be in a header file on its own and included as needed.

With classes defined:

class CLIPPER2_DLL Clipper2Class ....

When building the define CLIPPER2_DLL_EXPORT is declared. When linking to a library the define is not set.

On Linux the classes and methods will also be exported correctly when the option -fvisibility=hidden is used when creating a shared library.

Also see #169

@AngusJohnson
Copy link
Owner

AngusJohnson commented Oct 4, 2022

Following up on my question here... I'm still not persuaded that exporting classes is better that exporting well designed functions.
For example ISTM that the following function would provide all the functionality of directly accessing a Clipper64 object:

  static const unsigned CLIP_FLAG_REVERSE_SOLUTION = 1;
  static const unsigned CLIP_FLAG_REMOVE_COLLINEAR = 2;

  inline bool BooleanOp(ClipType cliptype, FillRule fillrule,
    const Paths64& subjects, const Paths64& open_subjects, const Paths64& clips,
    Paths64& closed_solutions, Paths64& open_solutions, unsigned clip_flags = 0)
  {
    Clipper64 clipper;
    clipper.AddSubject(subjects);
    clipper.AddOpenSubject(open_subjects);
    clipper.AddClip(clips);
    clipper.PreserveCollinear = !(clip_flags & 2);
    clipper.ReverseSolution = (clip_flags & 1);
    return clipper.Execute(cliptype, fillrule, closed_solutions, open_solutions);
  }

Of course there would need to be a similar function that returned PolyTree64, and the same for ClipperD.

@sergey-239
Copy link
Contributor

sergey-239 commented Oct 4, 2022

May I add my two cents, please?

I don't know how/is the version compatibility check is done in Windows world nowadays and was it ever implemented in MS C++ compiler system-wide. so to say (last time I coded something for Windows in the beginning of zeroes).

Nevertheless, I don't see a problem to implement such a check: there is a number of ways to call a function before Main in C++. So that function defined inside clipper2.hpp and only compiled during builds of 'importing' applications (#ifdef-ed), so it calls a getVersion function from the dll and aborts the app execution on version mismatch.
For the unix world this is done by .so versioning, thus no need for runtime check.

As for defining wrappers - it would only increase code maintenance difficulty: every time you change any method declaration to be exported you have to change the interface function. So, this is really unnecessary level of indirection and an additional source of possible bugs.

@AngusJohnson
Copy link
Owner

AngusJohnson commented Oct 4, 2022

As for defining wrappers - it would only increase code maintenance difficulty

Well I'm certainly motivated to avoid code maintenance as much as possible. And part of that is keeping the code as simple as possible. Anyhow, I'm not against exporting classes in principle, I'm just hoping for a dll/so solution that works well on all platforms.

@reunanen
Copy link
Contributor

reunanen commented Oct 4, 2022

When Clipper has a permissive license and compiles to a reasonably small binary, what exactly is the use case for a Clipper.dll on Windows?

@rs0xFFFF
Copy link

rs0xFFFF commented Oct 4, 2022

When Clipper has a permissive license and compiles to a reasonably small binary, what exactly is the use case for a Clipper.dll on Windows?

The C implementation is the fastest. You could convert this into a DLL. Delphi people can then just use these dll and take advantage of the speed.

@reunanen
Copy link
Contributor

reunanen commented Oct 4, 2022

But Delphi surely can't import C++ classes? So for that use case, there should be a plain C API, right?

@rs0xFFFF
Copy link

rs0xFFFF commented Oct 4, 2022

Yep!

@AngusJohnson
Copy link
Owner

But Delphi surely can't import C++ classes? So for that use case, there should be a plain C API, right?

And why I'm still (very slightly) leaning toward function wrappers instead of exposing classes.

@sergey-239
Copy link
Contributor

But Delphi surely can't import C++ classes? So for that use case, there should be a plain C API, right?

And why I'm still (very slightly) leaning toward function wrappers instead of exposing classes.

Yes, this changes the picture.
Btw, the same dll may expose C and C++ symbols at the same time, so basically, Delphi library becomes just a binding...

@AngusJohnson
Copy link
Owner

I've just added a new clipper.export.h file to create DLL/so files and a sample delphi app to test.
I see this file as a proof of concept rather than a definitive response to the above discussion.

@rs0xFFFF
Copy link

Seems to be a 32 bit problem.
DLL_32bit

@AngusJohnson
Copy link
Owner

I can't duplicate this. Are you using the latest commit?

procedure DoRandomStressTest(maxCnt: integer);
var
  i: integer;
  sub, clp: TPathsD;
  csub_local, cclp_local: CPathsD;
  csol_extern, csolo_extern: CPathsD;
begin
  csolo_extern := nil;
  SetLength(sub, 1);
  SetLength(clp, 1);

  for i := 1 to maxCnt do
  begin
    sub[0] := MakeRandomPathD(400, 400, 50);
    clp[0] := MakeRandomPathD(400, 400, 50);
    csub_local := TPathsDToCPathsD(sub);
    cclp_local := TPathsDToCPathsD(clp);

    BooleanOpD(Uint8(TClipType.ctIntersection),
      Uint8(TFillRule.frNonZero),
      csub_local, nil, cclp_local,
      csol_extern, csolo_extern);

    DisposeLocalCPathsD(csub_local);
    DisposeLocalCPathsD(cclp_local);
    DisposeExportedCPathsD(csol_extern);
    DisposeExportedCPathsD(csolo_extern);
    if i mod 100 = 0 then Write('.');    
  end;
  WriteLn(#10'Passed!');
end;

var
  s: string;
begin
  Randomize;
  DoRandomStressTest(10000);
  ReadLn(s);
end;

@rs0xFFFF
Copy link

Yes, I do
Clipper2_32.zip

@AngusJohnson
Copy link
Owner

AngusJohnson commented Oct 27, 2022

Seems to be a 32 bit problem.

OK, I missed that bit.

Edit: cdecl;
I'll upload the full fix shortly.

@AngusJohnson
Copy link
Owner

Fix just uploaded.

@rs0xFFFF
Copy link

Hi Angus, thanks for the correction. Does the benchmark mean (4000 edges) that the C implementation is 6x faster than Delphi? Bad time for Delphi? :-(
benchmark

@AngusJohnson
Copy link
Owner

AngusJohnson commented Oct 27, 2022

Hi Angus, thanks for the correction. Does the benchmark mean (4000 edges) that the C implementation is 6x faster than Delphi? Bad time for Delphi? :-(

Firstly, the performance test I've coded in the sample apps intentionally creates multiple intersections at the same locations as a kind of stress test of the clipping algorithm. And this also significantly slows clipping. This is important because it's not just the number of edges that affects timing, but the size of the random polygons too. Larger polygons will have fewer co-located intersections so clipping will be faster. Perhaps surprisingly ClipperD performance will usually appear to be much faster than Clipper64, but this is simply because ClipperD obects by default scale their paths *100. Consequently their polygons are much bigger (during clipping) and will have fewer co-located intersections. IOW, make sure you're comparing Clipper64 with Clipper64 (or comparing Clipper64 with ClipperD without scaling) and make sure the random polygons are generated into the same regions.

@rs0xFFFF
Copy link

Okay, understood, of course I have to take that into account.

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

No branches or pull requests

5 participants