search

symforce-org / symforce

Fast symbolic computation, code generation, and nonlinear optimization for robotics
https://symforce.org
Apache License 2.0
1.44k stars 147 forks source link

Centralize code for creating new code generation backends #158

Closed hmartiro closed 2 years ago

hmartiro commented 2 years ago

Move almost all backend-specific code into a sub-package of symforce/codegen/backends, rather than being scattered around multiple core files. This includes the code printer, codegen config, and a bunch of switch statements around the codegen machinery. The goal is to centralize and make it cleaner to add new codegen backends. These changes were made in parallel with adding a javascript backend, which will come in a follow-on review.

See this README for the steps to add a backend after this review.

There are still several ways to improve code quality and reorganize backend-specific code, but this is a step forward.

TODO: Regenerate all templates, since the template path changed (written to preamble).

hmartiro commented 2 years ago

Here's what it looks like to add a language after this review: https://github.com/symforce-org/symforce/pull/159

hayk-skydio commented 2 years ago

@gcross-zipline since you're working on adding Rust bindings, please see this review which simplifies things. We should aim to rebase on top of this once it merges, so it looks like #159.

hayk-skydio commented 2 years ago

Not sure about the name "backends" for target languages, since we also use that for symbolic backends. Could call them "targets"?

It's a good callout. I feel like this use of backend is pretty similar to LLVM's terminology which makes me want to keep it. Target is fine, but I'm just having trouble getting behind it. I wonder if the sympy/symengine thing could be called something else like "get_sympy". Now that i think about it symforce.set_backend does make me think it's setting the language it's going to generate to

aaron-skydio commented 2 years ago

Yeah I'm down with calling these backends and renaming sympy/symengine, maybe call that set_symbolic_api? Worried that set_sympy will be confusing between whether it "sets the sympy we're using" or "sets symforce to use sympy (as opposed to symengine)"

hayk-skydio commented 2 years ago

Yeah I'm down with calling these backends and renaming sympy/symengine, maybe call that set_symbolic_api? Worried that set_sympy will be confusing between whether it "sets the sympy we're using" or "sets symforce to use sympy (as opposed to symengine)"

Okay, doing that.