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

Made some changes to complex.h and assert.h docs and a few improved styles #3

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

Conversation

Himujjal
Copy link
Contributor

@Himujjal Himujjal commented Apr 3, 2021

Hey, can you check out this and comment on if the doc format (examples over function signatures) style is good.
https://zig-community.github.io/libc-to-zig/#/headers/complex_h


For the following C code, which will immediately abort the program if the condition is false.

```c
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add an #include <assert.h> here, just used to seeing it in manpages, same thing for importing std in the zig code example. A bit redundant but it allows for people to copy paste code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Will do that.

Also I will add the following scripts for testing Zig & C code so that any future code reference will be always correct.

https://github.com/Sobeston/ziglearn/blob/master/do_tests.zig
https://github.com/Sobeston/ziglearn/blob/master/test-out.zig

Will also write a similar zig "script" for C code too in future too.

assert(x >= 0.0); // undefined behaviour
```

This will invoke an undefined behaviour when the condition is `false`. Checkout [std.debug.assert](https://ziglang.org/documentation/master/std/#std;debug.assert)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a great spot to explain undefined behavior in zig and how it's caught in debug and release-safe, but optimized out of release-fast and release-small, there's a good explanation here https://github.com/ziglang/zig/blob/6fc822a9481c0d2c8b37f26f41be603dd77ab5a4/lib/std/debug.zig#L222

it's so simple that we might want to include it's implementation too.

I would also move the static assertion code up here and add a comptime assert example. (static_assert is also requires C11 iirc so that's important too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. unreachable to be introduced and more on static_assert & comptime assert


## [Zig Complex Type][21]

In Zig complex numbers are represented as `Complex` type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should note what the underlying number type is used in Complex

Copy link
Contributor Author

@Himujjal Himujjal Apr 3, 2021

Choose a reason for hiding this comment

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

Complex types take in any number type (f64, f32) i guess as a generic. Didn't really check the source code. But sure, I will add it.

const z2 = Complex { re: 0, im: 2.0 };

test "complex multiplication" {
const zSquared: Complex = complex.mul(z1, z2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't need to declare the zSquared type here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh. type inference. cool. will remove this!

```zig
const assert = std.debug.assert;
// ...
const x: double = -1.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's no double in zig, instead it would be f64 (same thing for int used elsewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh. sorry about that. i will fix that.

std.testing.expectApproxEqAbs(x1, x2, 0.0000001); // floating point number comparison with tolerance

const x3: int = 1;
const x4: int = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i32 should replace int

!> [cexpf][3] & [cexpl][3] are two variants in C that take `float` and `long double` respectively in C. In Zig those variants
are handled quite well by Zig generics.

<!-- C reference links -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

love all the reference links

@Himujjal
Copy link
Contributor Author

Himujjal commented Apr 3, 2021

Thanks for the review. I will change the stuff mentioned above. Make a PR again tomorrow.

@mattnite
Copy link
Collaborator

@Himujjal how's it coming?

@Himujjal
Copy link
Contributor Author

Hey. Totally got out of my mind, this project. I will put up the changes sure.

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