F2I-Consulting / fesapi

DevKit for ENERGISTICS™ data standards (mainly RESQML™), multi-languages (C++, Java, C#, Python)
Apache License 2.0
32 stars 24 forks source link

public methods lacking DLL_IMPORT_OR_EXPORT #300

Closed trebahl closed 2 years ago

trebahl commented 2 years ago

Is your feature request related to a problem? Please describe. Some public non inlined methods don't have a DLL_IMPORT_OR_EXPORT. This prevents from using them in windows.

The cases I stumbled upon are:

Describe the solution you'd like

Removing the existing DLL_IMPORT_OR_EXPORT, then adding DLL_IMPORT_OR_EXPORT to the classes, like:

class DLL_IMPORT_OR_EXPORT AbstractGridRepresentation { .... }

Describe alternatives you've considered Adding the missing DLL_IMPORT_OR_EXPORT one by one.

philippeVerney commented 2 years ago

Hi @trebahl,

Thanks for raising this issue.

"Public" does not mean "exported". FESAPI does have public methods which are not intended to be exported/used by final users. I agree that FESAPI public methods are by default all exported on Linux system and not on Windows system which may create some inconsistencies.

Initially, whole FESAPI classes were exported but it had created a lot of warnings. We have thought cleaner (even if more work for us) and more granular to selectively put DLL_IMPORT_OR_EXPORT on each method. I think the warnings were related to this : https://stackoverflow.com/questions/767579/exporting-classes-containing-std-objects-vector-map-etc-from-a-dll I am not telling that we cannot put DLL_IMPORT_OR_EXPORT on classes but it does not look to us that this would be cleaner, that this would be more precise.

trebahl commented 2 years ago

Thanks for your quick answer. I understand that dors and loadTargetRelationships don't need to be exported (I just mentionned them because I saw them not exported, but getChildGridSet was the method on this class that I was trying to use).

However, regarding Trigonometry.h, I think at least convertToRadians should be exported. This function seems important to me in order to make use of AbstractLocal3dCrs;;getArealRotation.

philippeVerney commented 2 years ago

I think I am going to make Trigonometry a header file only. This would :

About the third bullet above, FESAPI would really like to provide such functionality i.e. uom conversion but it is not done yet and would require a significant amount of time. Indeed, we could convert plane angle to radian, but also to angle degree and to so much other uom. If we would provide such functionality for plane angle, why not to provide it for lenght, weight, volume, etc... ? Though we really would like to work on that, I don't think we currently have the resources for it.