geogram
geogram copied to clipboard
Make geogram initialization less intrusive.
Currently geogram requires a call to GEO::initialize(), which is very intrusive:
- Sets its own env variable
LC_NUMERICon Unix system - Register its own
atexit()function. - Process::initialize will install new signal handlers by default
- The initialization function cannot be called concurrently either
- Etc.
Ideally such an initialization function should not be needed for a library integrated in a larger system. My recommendation:
- Use a single "Context" object to register global objects that are necessary (e.g. io plugins).
- Use Meyer’s Singleton to ensure thread-safe initialization and proper termination functions are called at program exit, without a specific
atexit()function. - Remove all intrusive registrations by default (signals, env variables, fpe, etc.)
- Remove the need to call
GEO::initialize()altogether (e.g. have each system call their own singleton struct as needed, or rework systems to remove the need for globals altogether).
There are other very annoying behavior with the CmdLine args. For example, Delaunay::create() tries to retrieve CmdLine::get_arg("algo:delaunay"), but this arg is only defined if one calls import_arg_group_algo(), which is not something that GEO::intialize() seems to be doing by default. This sort of deep dependency to obscure command-line args is scattered all over the place, which leads to crashes if one does not initialize geogram carefully.
A much better approach would be to default-initialize simple parameter structs in various headers, and only tie that to actual cmd line args in Geogram's applications. I'm a big fan of CLI11's API for doing this (e.g. see this example in Lagrange).
- 100% agree about most of the things.
- about Meyer's singletons, I was using that in the past (and I'm still using that in Graphite. What I don't like is that it is sometimes difficult to know in which order the destructors of the global objects are called. I found that a plain old non-magical
initialize()/terminate()pair of functions was clearer, but you are not the only one complaining about that so I should do something ... - what is the problem with registering functions with
atexit()? (there is a limited number of functions that one can register withatexit(), but it is still a large number I think)
Ideally, a library shouldn't use any global at all. I'd prefer to have to manually create a Context object, and pass it around as needed, but I recognize that having "some" globals can be useful (e.g. logging). I think there are a lot of patterns in Geogram that could be reworked to remove the need for globals altogether though. E.g. the "IO" stuff could hardcode the list of default readers/writers available, and provide a mechanism to hook up additional ones (maybe through some simple polymorphism/dispatch mechanism). The CmdLine stuff can be removed and each algorithm can use a dedicated parameter struct (optional and populated with sane default args), etc.
Regarding the atexit stuff, I'm not sure how well this works with dynamically loaded shared library (e.g. a plugin in Blender). But in any case, I'd much prefer to have the client call the terminate() function manually as they want, rather than silently registering it through atexit() (which is guaranteed to handle at least 32 functions, but not more).