USF-OS / FogOS

Other
1 stars 24 forks source link

Math Library #47

Open nogagottlieb opened 4 months ago

nogagottlieb commented 4 months ago

Since xv6 does not support the print of floats, we created another directory named math. To run it, just cd to math and type make and it will automatically compile and run our code. We use scanf for testing.

In the math directory, we created math.h, math.c, test.c, and Makefile. Functions our math library supports: osSqrt, osPow, osExp, osFloor, osCeil, osClosestInteger.

Jsk07211 commented 4 months ago

Emmanuel and I (Jaz) will do a code review for this :)

ghimirelava commented 3 months ago

Code Review: I thought your code was very well written. The code itself was very clean and the functions have good documentation.

Improvements:

emuwang commented 3 months ago

Hi everyone,

Just like the previous code review mentioned, the code for math functions are well written and easy to read, with good documentation.

Suggestions/Observations

1. The default math functions take in and return double

For example, the man page for pow shows that there are different variants for double, float and long double:

double pow(double x, double y)
float powf(float x, float y)
long double powl(long double x, long double y)

Fixes for this are relatively easy as you can just swap float to double in math.c and math.h and it should work as intended. Additionally, you could create powf, sqrtf, etc. and that would expand the scope.

2. printf for double/float

I noticed that there is a printf.c file under the kernel directory (kernel/printf.c) and that might be where xv6 takes printf from...? I played around with that printf.c file so that %f printing for double/float works but it was still throwing a usertrap mainly because I haven't fully figured out how printf is written in xv6. While this may not be in the scope for your pull request, it might be relevant in order for math functions to work when you run qemu.

3. closestInt

closestInt is not part of the math.h library (if I'm not wrong) but you could keep it around since it can come in handy!

4. int limit

I tried out some test cases that exceed the int limit just as edge cases and it outputs a negative number. While I'm not too sure on the fix for this, it could be worth looking into the source code for the math functions to see how they handle it. image_2024-03-05_161103430

Overall

Really well done functions with some big brain logic that took me some time to comprehend. Well done!

malensek commented 2 months ago

Nice work. Hopefully in the future we can add the rest of the functionality to get this merged in. :-)