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

Structure restructuring repo #28

Merged
merged 15 commits into from
Nov 26, 2021
Merged

Conversation

Simon-Stone
Copy link
Contributor

No description provided.

Copy link
Collaborator

@paul-krug paul-krug left a comment

Choose a reason for hiding this comment

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

Looks good generally, but I do not like the way include/ is split up into Backend and VocalTractLabApi, since the source files are not split up either.
In any way, a more fitting name for the api files would be API_C, as we may add a c++ api in the future. For the backend, I'd prefer lowercases, backend.

@@ -1,9 +1,66 @@
# CMakeList.txt : CMake project for VocalTractLabApi, include source and define
# project specific logic here.
#
cmake_minimum_required (VERSION 3.8)
cmake_minimum_required (VERSION 3.13)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the a specific reason that makes this change possible?

Copy link
Contributor Author

@Simon-Stone Simon-Stone Nov 26, 2021

Choose a reason for hiding this comment

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

Looks good generally, but I do not like the way include/ is split up into Backend and VocalTractLabApi, since the source files are not split up either. In any way, a more fitting name for the api files would be API_C, as we may add a c++ api in the future. For the backend, I'd prefer lowercases, backend.

Splitting up the header files into Backend and VocalTractLabApi folders is best practice to avoid ambiguities. Let's say we were to use some other library at some point that included a header file with a name that already exists in the backend or API. If the header files were at the root level of include/, there was no way to unambiguously choose the right one to include. Using the subfolders, you just set the include/ directory and then include like so:

#include "Backend/Glottis.h"
#include "SomeOtherLib/Glottis.h

(This of course brings to attention the fact that we don't have namespaces in our codespace, but that is a topic for another time)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The uppercase Backend is conforming to Peter's conventions. As is the name VocalTractLabApi.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, there was a reason why CMake 3.13 is required now, but I'm gonna have to check which routine made that necessary.

Copy link
Collaborator

@paul-krug paul-krug Nov 26, 2021

Choose a reason for hiding this comment

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

Splitting up the header files into Backend and VocalTractLabApi folders is best practice to avoid ambiguities. Let's say we were to use some other library at some point that included a header file with a name that already exists in the backend or API. If the header files were at the root level of include/, there was no way to unambiguously choose the right one to include. Using the subfolders, you just set the include/ directory and then include like so:

I get that, but why are the source files not split up, its the same situation isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would still be VocalTractLabBackend.dll or *.so, then.

Copy link
Contributor Author

@Simon-Stone Simon-Stone Nov 26, 2021

Choose a reason for hiding this comment

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

Hm, maybe we should change the name of the Backend folder(s) to VocalTractLabBackend? Seems more consistent compared to the API folders. And just Backend seems very generic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, maybe we should change the name of the Backend folder(s) to VocalTractLabBackend? Seems more consistent compared to the API folders. And just Backend seems very generic.

Yes, VocalTractLabBackend sounds better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would still be VocalTractLabBackend.dll or *.so, then.

True, you are right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I thought that we could simply wrap the VocalTractLabApi header around the static VocalTractLabBackend lib and build a shared lib from it, but apparently it is not as straight-forward. The recommended way of doing it is to include all the backend sources again explicitly when building the shared API library.

Copy link
Collaborator

@paul-krug paul-krug left a comment

Choose a reason for hiding this comment

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

JD2 should be removed from resources, it is not possible to load this file anymore.

@Simon-Stone Simon-Stone merged commit c5ed021 into main Nov 26, 2021
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