geogram icon indicating copy to clipboard operation
geogram copied to clipboard

Make geogram initialization less intrusive.

Open jdumas opened this issue 2 years ago • 3 comments

Currently geogram requires a call to GEO::initialize(), which is very intrusive:

  • Sets its own env variable LC_NUMERIC on 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).

jdumas avatar Mar 01 '23 17:03 jdumas

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).

jdumas avatar Mar 01 '23 17:03 jdumas

  • 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 with atexit(), but it is still a large number I think)

BrunoLevy avatar Mar 02 '23 22:03 BrunoLevy

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).

jdumas avatar Mar 02 '23 23:03 jdumas