Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework ffi with new logging, error handling and some improvements in arch and safe #10

Open
wants to merge 77 commits into
base: main
Choose a base branch
from

Conversation

uselessgoddess
Copy link
Member

No description provided.

@uselessgoddess uselessgoddess changed the title Remove ffi with new logging and some improvements in arch and safe Rework ffi with new logging and some improvements in arch and safe Aug 14, 2022
@uselessgoddess
Copy link
Member Author

It also may solve #2 issue

- use more debug logs
- use more high-level types
- fix useless code in ffi bindings
- add function to work with error data (incomplete)
- improve `doublets`, `platform-data` to make it compatible with new changes
@uselessgoddess uselessgoddess changed the title Rework ffi with new logging and some improvements in arch and safe Rework ffi with new logging, error handling and some improvements in arch and safe Aug 18, 2022
Copy link
Member Author

@uselessgoddess uselessgoddess left a comment

Choose a reason for hiding this comment

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

I did a little review

Comment on lines +23 to +27
tracing::error!("SOMETHING IS SERIOUSLY WRONG!!!");
tracing::warn!("important informational messages; might indicate an error");
tracing::info!("general informational messages relevant to users");
tracing::debug!("diagnostics used for internal debugging of a library or application");
tracing::trace!("very verbose diagnostic events");
Copy link
Member Author

Choose a reason for hiding this comment

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

Move logs out unsafe context. It's confusing

Comment on lines 26 to 30
unsafe {
let handler = &mut *(ctx as *mut F);
(*handler)(before, after);
Flow::Continue
}
Copy link
Member Author

Choose a reason for hiding this comment

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

unsafe context is too long

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
unsafe {
let handler = &mut *(ctx as *mut F);
(*handler)(before, after);
Flow::Continue
}
let handler = unsafe { &mut *(ctx as *mut F) };
(*handler)(before, after);
Flow::Continue

Comment on lines 33 to 39
unsafe fn magic_create<F>(ptr: *mut c_void, handler: F)
where
F: FnMut(Link<u64>, Link<u64>),
{
let ctx = &mut (ptr, handler);
let _ = create(ptr, null(), 0, ctx as *mut _ as *mut _, create_cb::<F>);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We could supplement this example almost to the level of real use

let path = CString::new("doublets.links").unwrap();
let mut store = doublets_create_united_store_u64(
path.as_ptr(),
Constants::from(LinksConstants::external()),
Copy link
Member Author

Choose a reason for hiding this comment

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

Add ffi functions to create external/internal constants

Comment on lines 12 to 16
if *ctx % 2 == 0 {
print!("{str}");
} else {
eprint!("{str}");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between print and eprint is not noticeable. Use something more different

Copy link
Member

Choose a reason for hiding this comment

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

More panic!

Copy link
Member Author

Choose a reason for hiding this comment

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

Bro, panic abort my example :(

doublets-ffi/src/store.rs Outdated Show resolved Hide resolved
doublets-ffi/src/store.rs Outdated Show resolved Hide resolved
doublets-ffi/src/store.rs Outdated Show resolved Hide resolved

fn place_error<T: LinkType>(err: Error<T>) {
// It can be very expensive to handle each error
debug!(op_error = % err);
Copy link
Member Author

Choose a reason for hiding this comment

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

op_error? Think of something better

doublets-ffi/src/store.rs Outdated Show resolved Hide resolved
- Fix forgotten `each` in moder style
- Rename `DoubletsErrorKind` to `DoubletsResultKind` (it must contain `Continue`/`Break`)
- improve error handling for create store (and fix example)
- Fix forgotten `each` in moder style
- Rename `DoubletsErrorKind` to `DoubletsResultKind` (it must contain `Continue`/`Break`)
- improve error handling for create store (and fix example)
- use modern syntax:
  ```rust
     #[ffi::specialize_for(
         types(
             u8  => "u8",
             u16 => "uint16",
             u32 => "uint",
             u64 => "ll",
         ),
         name = "...*",
     )]
  ```
- add warnings handler
- use more beautiful code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants