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

feat: implement require macros and fix log docs #493

Merged
merged 14 commits into from
Aug 16, 2021
Merged

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Jul 25, 2021

Basically want to provide macros to mimic similar behaviour to the rust assert! macros without it adding extra rust details and file information about the assertion.

Built on #492 to avoid conflicts and use the correct method signature, but can switch if we want this and not that PR.

Optional which opinions would be appreciated:

  • Should the no message call panic() syscall directly? I assumed not because it seems like a micro-optimization that would be hard to debug by default. Pinpointing that it's a require statement might help developers

Base automatically changed from austin/panic_str to master August 3, 2021 14:34
#[macro_export]
macro_rules! require {
($cond:expr $(,)?) => {
if !$cond {
Copy link
Contributor

Choose a reason for hiding this comment

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

cool, seems like these will be less costly on CPU cycles than asserts, is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that will necessarily be the case, but it should reduce the code size from not including file/line numbers of where the assert is. This also depends on the use case, though.

@austinabell
Copy link
Contributor Author

@matklad would love a quick opinion on this if you think this is a good direction for having this type of assertion available without the Rust internals.

Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Couple of thoughts:

  • I wonder if we should just call this assert. The require name doesn't really point to the difference
  • You probably don't need _eq/_neq versions -- they exist solely to provide better assertion messages. we don't allow assertion message, assert!(x == y) is enough.
  • Would be cool to get some specific numbers on code saving here: I know that fmt is heavy in principle, but I don't have a gut feeling for how much.

Overall seems like a good idea!

};
($left:expr, $right:expr, $message:expr $(,)?) => {
if $left != $right {
$crate::env::panic_str($message.as_ref())
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the rationale was that I wanted the ability to supply something that wasn't just a static string or if keeping expr then forcing the user to pass in a &str, in place of not having the formatting machinery.

I was thinking it would be used as the log! macro where it allows String, &str, or anything that implements AsRef<str>. I suppose all we want here is to allow owned or str references, so I think I will take this suggestion because it's unlikely someone will assert with the message being a single struct

@austinabell
Copy link
Contributor Author

austinabell commented Aug 6, 2021

Couple of thoughts:

  • I wonder if we should just call this assert. The require name doesn't really point to the difference

I personally think that keeping it named require to avoid collision with the std prelude and making it clear that it performs a different function is good rather than having unexpected side effects if someone does/doesn't import the near_sdk::assert, do you have a strong opinion on naming it assert?

  • You probably don't need _eq/_neq versions -- they exist solely to provide better assertion messages. we don't allow assertion message, assert!(x == y) is enough.

Good point! I'll remove those for now. My rationale was that it would be easier for migration to these from the assert variants, but can always just add later if we want

  • Would be cool to get some specific numbers on code saving here: I know that fmt is heavy in principle, but I don't have a gut feeling for how much.

So it's hard to tell, since most of fmt is still pulled in for all contracts using the sdk, but roughly can see the contract size changes in #514 (since now we are tying the example builds with docker to the CI checks)

@austinabell
Copy link
Contributor Author

Just in case of ambiguity, I'm assuming your approval as invalid @mikedotexe because I've made changes since your approval.

Copy link
Contributor

@willemneal willemneal left a comment

Choose a reason for hiding this comment

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

Looks good to me. I tried to think of why require wouldn't be a good name (since it's overloaded with node's old way of importing modules), but given that they are moving away from that to import * from * I think it's good.

@austinabell austinabell merged commit 1a28451 into master Aug 16, 2021
@austinabell austinabell deleted the austin/require branch August 16, 2021 15:57
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.

4 participants