Skip to content

Conversation

@fhshaik
Copy link

@fhshaik fhshaik commented Sep 27, 2024

Implementation of software based floating point emulation and Calculator Utility to test implemented floating point numbers. For calculator utility, the format is:

Floating point numbers must be inputted in a decimal format, for example "1330.39" or "2929.0." Keep this in mind. Allowed operations are <+, -, /, *> (addition, subtraction, division, multiplication). Future operations like square roots and exponentials are planned. These floating-point numbers DO NOT use IEEE standard for floating point numbers. As a result, we have a wider range of floating-point numbers. I hope future students who need floating point numbers for projects will enjoy what I made.

@dtruong33
Copy link

I can code review this

@ChristopherBrower
Copy link

I can code review this as well

@ChristopherBrower
Copy link

High Level Checks:

  • PR fully resolves the task/issue
    - Seems to address the issue, though testing of the code to make sure it actually works happens in later part.
  • Does the PR have tests?
    - While it doesn't have specific tests, it has an easy way to test, with the calculator.
  • Does the PR follow project code formatting standards?
    - The formatting seems fine, though perhaps slightly different from the existing code in format, it is not really a big difference, and something a code formatter could change throughout if needed.
  • Does the OS still compile and run?
    - The OS will compile and boot when you build it.
  • Is documentation included?
    - While there is some comments that seem to partially document the code, I would prefer it if it had more documentation, especially around conversion to floats, and calculation with them, as that seems to be more complex at times.

Code Checks:

  • Does the code achieve its intended purpose?
    - the code seems to work, though perhaps is not the easiest to work with sometimes.

  • Were all edge cases considered?
    - if calculator is not provided any arguments, it gives a user trap.
    - if you give it an expression, with no spaces, it doesn't do anything
    - it does not handle integers nicely, so if you forget to add .0, it can give strange results

  • Is the code organized and maintainable?
    - It seems fine, perhaps there could be less bit shifts, or they could be explained better, but it is probably fine. perhaps adding more comments would help.

  • Does the code maintain the same level of performance? (no performance regressions)
    - no performance problems I noticed.

Overall, this code seems good, though I would recommend a few changes. First, make sure to add more documentation, perhaps in the form of comments explaining what is happening for some of the bit shifts. Additionally, modify the calculator so it does not give a user trap if there are no arguments provided. Relatedly, if the program is not provided with all the required arguments, output a helpful message telling users what inputs are expected, rather than providing no output at all. Finally, I would make the calculator handle integers better, compared to how it is currently done. For example, with inputs like 127.091 * 3, the result given was 55.557729, which is incorrect; the result should be closer to what is given when both numbers are floats(e.g., 127.091 * 3.0 gives 381.273). While this is not a major issue since you specifically mentioned it, changing it would improve the usability, by addressing a minor inconvenience people might run into.

@dtruong33
Copy link

dtruong33 commented Oct 1, 2024

High Level Checks:

  • PR fully resolves the task/issue
    --- The PR fully implements a basic calculator using floats
  • Does the PR have tests?
    --- Tests not needed
  • Does the PR follow project code formatting standards?
    --- No major formatting issues
  • Does the OS still compile and run?
    --- OS compiles and runs like normal
  • Is documentation included?
    --- No Javadocs

Code Checks:

  • Does the code achieve its intended purpose?
    --- The calculator functions as expected with floats
  • Were all edge cases considered?
    --- Several cases where the calculator freezes
  • Is the code organized and maintainable?
    --- The code is well organized
  • Does the code maintain the same level of performance? (no performance regressions)
    --- The code does not noticeably impact performance nor does the calculator heavily impact performance

All in all the PR successfully implements a basic float calculator utility in XV6 with the usual calculator operations. The code as a whole is well formatted with only minor deviations from XV6's formatting but it is consistent. It is a good addition to XV6 and easy to see its benefits. The only improvements I can see that can be added are documentation, tests, and fixing bugs. Javadocs can be added to better explain how functions work. Finally, there are several bugs that cause the calculator to freeze and lock up, such as if the result is an integer (i.e 9.9+1.1) and if 0.0 is included in multiplication or division operation. But overall, this is a great addition and utility for XV6.

@malensek
Copy link
Contributor

malensek commented Oct 23, 2024

This is one that I wasn't sure if it would work out given the complexity, but you did it! Very cool.

I do see the rough edges the CRs above found. The test program / calculator has some issues, but the library itself seems to be quite solid. Really, the main thing here would be fleshing out the calculator so it's a bit more fully featured (in its current state it feels more like a proof of concept), but that would be a great future project for someone else to pick up.

EDIT: I realize you have a pending commit to fix some of the issues. I'll check that out once it's up.

Very nice work. This was quite ambitious and a super cool contribution.

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