Manual body definitions #17
Conversation
Defining body structs manually rather than relying on generated code prevents circular compiler errors in the event of badly generated code that must be deleted, but is depended on by manual trait implementations, which must compile in order to rerun the generator.
.gitignore
Outdated
| .idea/ |
There was a problem hiding this comment.
I finally succeeded in gitignoring the idea files successfully. May it rest in peace. Sorry about the extra PR noise.
There was a problem hiding this comment.
Did not really care before. This is the nuclear option which is certainly valid but it seems that JetBrains recommends to keep some of these files under version control: https://intellij-support.jetbrains.com/hc/en-us/articles/206544839-How-to-manage-projects-under-Version-Control-Systems
There was a problem hiding this comment.
That's fair enough. It's a habitual response after years of culling .DS_Store and VSCode artifacts. I'm happy to default it.
| pub trait NaifId: Copy { | ||
| fn id() -> i32; | ||
| const ID: i32; |
There was a problem hiding this comment.
Given the opportunity, I refactored this to use the pattern we've adopted for RotationalElements.
| /// Expands to derivations of the fundamental traits every body must implement. | ||
| macro_rules! body { | ||
| ($i:ident, $naif_id:literal) => { | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] |
There was a problem hiding this comment.
It occurred to me that none of these structs need to be Clone or Copy, and nor do our trait implementations require these bounds, since empty structs have zero size. Rust effectively considers any operation on them (such as passing them to another function) to be a no-op. These can be removed in a small follow-up to avoid creating more noise here.
More on zero-sized types here: https://doc.rust-lang.org/nomicon/exotic-sizes.html#:~:text=feature%20for%20now.)-,Zero%20Sized%20Types%20(ZSTs),-Rust%20also%20allows
There was a problem hiding this comment.
Are you sure about this? I only added the annotations after running into trouble.
There was a problem hiding this comment.
Let me run some more experiments.
There was a problem hiding this comment.
So, the following compiles fine for me (where NaifId is defined without a Copy bound):
#[cfg(test)]
mod nocopy_tests {
use crate::bodies::NaifId;
struct SomeBody;
impl NaifId for SomeBody {
const ID: i32 = 1;
}
#[test]
fn sends_body() {
let some_body = SomeBody;
forwards_body(some_body);
forwards_body(SomeBody);
}
fn forwards_body(body: impl NaifId) {
receives_body(body);
}
fn receives_body(body: impl NaifId) {
println!("all good")
}
}BUT, removing Copy would absolutely cause us problems interacting with stdlib or third-party functions that require a Clone/Copy bound, so better to have it than not. As you were!
| @@ -202,53 +411,37 @@ pub trait RotationalElements: Copy { | |||
| mod tests { | |||
There was a problem hiding this comment.
This diff is super confusing because I moved the manual definition of Jupiter up a few lines, and the definition is huge. In short: all tests are now defined in terms of our manually defined Jupiter, decoupling them from the generated code.
| @@ -262,7 +262,12 @@ impl GetPolynomialCoefficients for CoefficientKernel<'_> { | |||
| })) | |||
| } | |||
| // Split the implicit pairs into two index-matched vecs. | |||
| Some(coefficients) => Ok(TokenizeableNutPrecCoefficients(unpair(coefficients))), | |||
| Some(coefficients) => { | |||
There was a problem hiding this comment.
Missed this conversion to radians previously!
crates/lox_core/src/bin/lox.rs
Outdated
|
|
||
| fn main() { | ||
| println!("Hello LOX"); | ||
| println!("{}", MercuryBarycenter::id()) | ||
| println!("{}", MercuryBarycenter::NAIF_ID) |
|
LGTM 👍 |
Refactors how empty body structs are defined, moving the responsibility from
lox_gentolox_core::bodies. This helps us to avoid circular compiler errors where generated files are malformed, but can't be regenerated without commenting out all crate code that depends on their body definitions.Additionally fixes a bug where nut prec coefficients weren't being converted to radians.