search

jamoma / JamomaCore

Jamoma Frameworks for Audio and Control Structure
Other
36 stars 14 forks source link

New extension loader code #333

Closed jcelerier closed 9 years ago

jcelerier commented 9 years ago

And other fixes for platforms other than OS X.

The huge diff is due to the libxml/libiconv upgrade. I also need to do it for portmidi (used by MidiLib and MIDI extensions), because the version in the Jamoma folder has been built with /MT (static runtime) and this is incompatible (and outright crashes) when trying to use it with the rest of Jamoma, libxml, & al which are built with /MD.

tap commented 9 years ago

From my email to the jamoma-devel list:

  1. It is a tremendous improvement to reduce the horrible code duplication we had to address the different platforms. Hallelujah!
  2. In TTExtensionLoader.h there is now the use of "#pragma once". In the past (years ago) this was not an option because it was not supported in GCC 2.8 as used by Xcode. Now it is supported by all modern compilers and I propose that we shift to this idiom rather than our old-fashioned header guards throughout all of Jamoma. Anyone opposed?
  3. There is code here which does not follow the usual Jamoma naming conventions. This not really the fault of anyone as we haven't really published anything about this -- and we have other places in the code that are less-than-rigorous about this.

The concern here is that it makes it slightly hard to understand what is going on if we start having diverging styles throughout the code. Some style really doesn't matter and is a preference (where to put { and } for example). But the names of functions/templates/classes should try to adhere our conventions.

For example: OperatingSystem should be TTOperatingSystem. This way we know immediately that this is something we've defined in Jamoma and it is not something we're pulling in from some library.

My apologies if this seems tedious, but things like this can bite us down the road if we don't pay attention to it now!

Similarly, loadClass() would benefit from name consideration. One thing to consider is that loading an extension is not the same as loading a class -- an extension may (and often does) contain many many classes.

  1. +void TTFoundationInit(const char* pathToBinaries, bool loadFromOtherPaths)

Without reading the source code, I find this bool "loadFromOtherPaths" to be a really confusing (or at least not self-documenting) name. But looking at this raises bigger questions...

In some of the new code we are using a vector of strings. It is very temping to say that TTFoundationInit() should be rewritten to accept such a vector, e.g. TTFoundationInit(TTStringVector pathsToExtensions).

That's also scary though. We've experienced (extreme) pain with std::string crossing the boundaries of binaries on the Windows platform. I'm not sure what the latest thoughts or developments are in that regard or if it has changed with C++11 and C++14.

Does anyone know?

jcelerier commented 9 years ago

Hello,

Thanks for merging. I haven't had the time to answer but I'll try to do it ASAP this week (and fix the style, etc. in subsequent commits.)

Best, Jean-Michaël