Skip to content

Conversation

@jcchu0
Copy link

@jcchu0 jcchu0 commented Sep 23, 2024

This pull request enhances the [cat] utility by implementing additional flags and improving the handling of non-printing characters. The following changes have been made:

support four new flags:
-n for adding line numbers and
-v for displaying non-printing characters in a visible format. The utility will read from files or standard input and process the content based on the provided flags.
-b flag to number non-blank lines only.
-E flag to display $ at the end of each line.

Testing for flag -v is done externally through the OS/user program as detailed in the cat.c program.

High Level Checks:

  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose?
  • Were all edge cases considered? (WIP)
  • Is the code organized and maintainable?
  • Could the code cause any performance regressions? (No)

@rnayakusf
Copy link

I can review this.

@ChristopherBrower
Copy link

I can code review this as well

@ChristopherBrower
Copy link

ChristopherBrower commented Sep 26, 2024

Code Review:
High Level Checks:

  • PR fully resolves the task/issue
    (The code looks to implement the features, though this item doesn't say anything about whether it works, that happens later)
  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
    (The formatting seems fine, while it slightly differs from the original formatting, it still is fine. While I don't really like switch statements, they are still ok to use.)
  • Does the OS still compile and run?
    (It builds and boots, and the test command works, though test.txt is not included, maybe fix that)
  • Is documentation included?
    (The code looks to have documentation in the form of comments.)

Code Checks:

  • Does the code achieve its intended purpose?
    (The Line numbers do not seem to print out properly with cat -n cat, instead printing %6d. This might be because this is not an implemented format for FogOS printf, so you might want to check. The -E flag prints the $ at the beginning of the next line, rather than on the end of the current line, presumably because it is printing without a newline, and adding it after the newline character of the line)
  • Were all edge cases considered?
    (Running cat with no args leads to it just showing output of whatever you type, making the os not really usable after it, which does not seem intended.)
  • Is the code organized and maintainable?
    (It overall looks fine though perhaps you could move the flag variables into main, that is probably not necessary.)
  • Does the code maintain the same level of performance? (no performance regressions)
    (It seems to run efficiently)

Overall this looks pretty good, though there are a few changes you could make. I would recommend handling the case where no file is passed, though that is a problem with the original cat implementation in FogOS. I would also recommend fixing the cat -n to actually print line numbers instead of %6d. Additionally, you should probably make the -E option put the $ at the end of the line instead of the beginning of the next one, or change the description to match what it does. Finally, as a future direction, you could add a -h flag, to output what each flag actually does.

@rnayakusf
Copy link

rnayakusf commented Sep 27, 2024

This is a good attempt, though it has a few issues. As mentioned, %6d is not supported by xv6's println, so the line numbers don't show up when using -n or -b. Also mentioned, The $ from -E are printed after the newline rather than before. Something I found was that if a nonblank line starts with whitespace (" there's text here"), -b doesn't number it.

That said, your variable names are descriptive and javadoc is helpful to understand the main cat function. The implementation is understandable, and the inclusion of the test program was nice, as it shows how the non-printing symbols are printed.

Also, don't worry about fixing what happens when you pass in no arguments to cat. It's standard behavior. :)

High Level Checks:

  • PR fully resolves the task/issue (It's provides an attempt at all the points mentioned, but falls short.)
  • Does the PR have tests?
  • Does the PR follow project code formatting standards?
  • Does the OS still compile and run?
  • Is documentation included?

Code Checks:

  • Does the code achieve its intended purpose? (As mentioned, there are still bugs that result in it not working correctly)
  • Were all edge cases considered? (See the -b bug I mentioned. I recommend using fgets to remedy it.)
  • Is the code organized and maintainable?
  • Does the code maintain the same level of performance? (A small hit since cat now reads each byte twice instead of once, but it's necessary for the new functionality)

Suggestions

  • Fix the bugs
  • Ignore newline for nonprinting symbols as it makes the output hard to read, the opposite intention of the -v flag. We have the -E flag for that, anyways.

@malensek
Copy link
Contributor

In this particular case the CRs above did all the work for me, mostly.

  • Why the uint8s for the options? I suppose if you are worried about efficiency you could use a struct with bitfields :-)
  • You should probably explain non-printing characters a bit more so future students understand.
  • Agreed with the second CR above, hanging and waiting for input is the right behavior, so keep that.

Overall, solid, but this needed to be tested more. You can earn +0.5 by fixing the suggestions in the CRs above.

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