Skip to content
This repository has been archived by the owner on Jan 15, 2022. It is now read-only.

Update to current Mentat interfaces #26

Closed
wants to merge 2 commits into from
Closed

Conversation

rnewman
Copy link

@rnewman rnewman commented Feb 1, 2018

Much less code, much less fragmented imports, fewer explicit dependencies. Tests pass.

Note that this temporarily points to my fix-the-world PR, which needs to land alongside this.

@rnewman rnewman self-assigned this Feb 1, 2018
}
}

impl ToTypedValue for Timespec {
fn to_typed_value(&self) -> TypedValue {
// TODO: shouldn't that be / 1000?!
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fluffyemily I didn't change the logic here, but superficially this looks like a bug…

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a bug. And quite plainly crashes the code. How has this not been found before? We store Timespec's all the time!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably you only ever test this in native code, and you build libtoodle.a in release mode, which I imagine permits overflow.

Copy link

@fluffyemily fluffyemily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to pretend to understand your use ordering (why is ffi_utils ordered with std and time, but store is ordered with items and labels?), but assuming there is some sort of logic to it, it has my rubber stamp.

@rnewman
Copy link
Author

rnewman commented Feb 1, 2018

The ordering I've been using is almost certainly not mine, and is loosely based on Java conventions:

At the top, general-purpose extern crates, in alphabetical order, newline-delimited if it makes macro_use easier to read.

#[macro_use] extern crate error_chain;

extern crate ffi_utils;
extern crate libc;
extern crate rusqlite;
extern crate time;
extern crate uuid;

Then "domain crates" — Mentat, in our case.

extern crate mentat;

Then subcrates — crates in the same workspace. (Rust might treat these specially in the future for traits, but it also makes it easier to find what uses what.)

extern crate store;

Then the use block, in this order, with chunks separated by newlines:

  • std (the standard library), alphabetically.
  • Crates, alphabetically, using same-line for modules (use ffi_utils::log) and the multiline {} syntax for symbols.
  • Domain crates.
  • Subcrates.
use std::ffi::CString;
use std::os::raw::c_char;
use std::str::FromStr;

use ffi_utils::log;
use ffi_utils::strings::{
    c_char_to_string,
    optional_timespec,
};

use libc::{
    c_int,
    time_t,
};

use time::{
    Timespec,
};

use mentat::{
    IntoResult,
    QueryExecutionResult,
    TypedValue,
    Uuid,
    Variable,
};

Then module imports and re-exports:

pub mod labels;
pub mod items;
pub mod errors;
pub mod ctypes;

Then module uses:

use errors as list_errors;

use labels::Label;

use items::{
    Item,
    Items,
};

use ctypes::{
    ItemC,
    ItemsC,
    ItemCList
};

use store::{
    Store,
    StoreConnection,
    ToInner,
    ToTypedValue,
};

Clearly I screwed up a little here by putting use store at the end — it's very hard to tell apart a same-crate module and a crate! — but I think you get the idea.

Naturally the mod/use/etc. sequence can be slightly perturbed by macro dependencies, which is a good reason to put macros in lib.rs.

@fluffyemily
Copy link

Hence my confusion as ffi_utils is a subcrate.

@rnewman
Copy link
Author

rnewman commented Feb 1, 2018

Very difficult to tell apart a subcrate from a dependency!

This is one reason we prefixed all of Mentat's subcrates.

@fluffyemily
Copy link

#28

@rnewman
Copy link
Author

rnewman commented Feb 1, 2018

bd5b1ea

@rnewman rnewman closed this Feb 1, 2018
@rnewman rnewman deleted the rnewman/mentat-0.6 branch February 1, 2018 17:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants