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

FreePascal language support #377

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

laheller
Copy link

As the title says. To make it happen at least on Windows (not tested on other platforms) I had to slightly modify the astronomy.h header in Astronomy Engine C/C++ library. This way the Pascal wrapper can "see" and call all the C functions that the C library binary Astronomy.dll exports. Relevant READMEs included everywhere.

@cosinekitty
Copy link
Owner

Hi @laheller, I have started looking at this PR. Thank you for volunteering to add support for Pascal. I mostly like your changes but there are a few things that need to be different:

  1. I would like to avoid including Visual Studio projects/solutions, at least in the source/c directory. I have had people ask to provide CMake, Visual Studio, and other build systems in Astronomy Engine, but I resist these requests because they multiply the complexity supporting the project for everyone. In principle, I try to resist including things that cannot be automatically tested when I push to GitHub. Tools keep changing, and once you support one person's favorite build environment, other people line up asking for theirs to be supported also. I'm trying to support the widest possible scope of applications, from microcontrollers to servers, so it's better not to be opinionated about how the code will be built.
  2. The DLL export prefix you added to astronomy.h is mostly OK, but it assumes that everyone using the code on Windows is going to use these functions from a DLL. I'm concerned that this will break the code for people already using it on Windows. I think it would be better for people who want DLL linkage to supply a preprocessor symbol like ASTRONOMY_ENGINE_DLL. The code can check for ASTRONOMY_ENGINE_DLL; if present, verify you are on Windows. If not Windows, use #error Unsupported platform for DLL linkage, otherwise do your stuff with defining the prefix. This approach requires Pascal users (or other cases where DLL linkage is desired) to opt-in by adding -D ASTRONOMY_ENGINE_DLL to their Makefile, Visual Studio project, CMake configuration, etc.
  3. The name _PREFIX is so generic that it could cause conflicts with other header files. I would prefer something like ASTRO_FUNC or something like that.

I would be happy to take your PR and make these changes myself, but I wanted to write to you first to make sure I'm not misunderstanding you. What do you think?

@laheller
Copy link
Author

laheller commented Jan 27, 2025

Hi @cosinekitty

OK, I understand your points. Since I mostly focused only on the Pascal side to make it work on Windows (I had no time to test on Linux) I did not even think about the things you mentioned.

So my proposals to your points:

  1. I would simply move the Visual Studio solution file (.sln) and project file (.vcxproj) to a separate folder, for example:
    astronomy/build/c/vs2022
    and change the .vcxproj file (it's an XML actually) accordingly, means the lines
    <ClCompile Include="astronomy.c" />
    and
    <ClInclude Include="astronomy.h" />
    to:
    <ClCompile Include="..\..\..\source\c\astronomy.c" />
    and
    <ClInclude Include="..\..\..\source\c\astronomy.h" />.
    The .sln file doesn't need to be changed as long as it's in the same folder as the .vcxproj file.

  2. but it assumes that everyone using the code on Windows is going to use these functions from a DLL.

That's not true. Even with this preprocessor symbol defined on Windows, you still can use astronomy functions either from a .DLL or directly complied them into a .EXE executable. Of course I tested both cases and both worked!

The other platforms, like Linux are not affected because of the condition
#ifdef _WIN32
Which is only defined on Windows platforms and only for Microsoft's Visual C++ compiler.

Anyway, feel free to change as you like. At the end it's your repo & your astronomy engine :-)

  1. Feel free to rename the _PREFIX to whatever better name, like _ASTRONOMY_EXPORT_FUNCTION_NAMES.

BR,

Ladislav

@laheller
Copy link
Author

laheller commented Feb 9, 2025

@cosinekitty
Any feedback on my last comment?

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.

2 participants