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

cat: Add the -n line numbering feature #24382

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PerrinJS
Copy link

Adds the -n line numbering feature to cat, with the same formatting as gnu's cat function.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 19, 2024
Copy link
Member

@LucasChollet LucasChollet left a comment

Choose a reason for hiding this comment

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

Welcome to the project!

It doesn't really matter, but you can put the function in an anonymous namespace above the main to avoid having a declaration.

And also, it feels like output_buffer_with_line_numbers can be written in a simpler way. Feel free to try something on that side, otherwise I will give it a shot on a future review.

Userland/Utilities/cat.cpp Outdated Show resolved Hide resolved
Userland/Utilities/cat.cpp Outdated Show resolved Hide resolved
Userland/Utilities/cat.cpp Outdated Show resolved Hide resolved
Adds the -n line numbering feature with the same formatting as gnu's cat
function.
buffer_itter++;

if (line_count == 1 && (buffer_itter == buffer_span.begin())) {
out(" {: >6}\t{:c}", line_count, curr_value);
Copy link
Member

Choose a reason for hiding this comment

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

{: >6} will take care of putting the spaces, no need to add some before

-out("   {: >6}\t{:c}", line_count, curr_value);
+out("{: >6}\t{:c}", line_count, curr_value);

out("{:c}", curr_value);
if (!buffer_itter.is_end()) {
// This matches the formatting of gnu's cat function
out(" {: >6}\t", line_count);
Copy link
Member

Choose a reason for hiding this comment

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

same here

auto curr_value = *buffer_itter;
buffer_itter++;

if (line_count == 1 && (buffer_itter == buffer_span.begin())) {
Copy link
Member

Choose a reason for hiding this comment

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

buffer_itter == buffer_span.begin()
This condition is never true as you always increment buffer_itter on the previous line.

Copy link
Member

Choose a reason for hiding this comment

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

This makes me have this wrong result:

$cat -n README.md 
# SerenityOS and Ladybird
        1	
        2	SerenityOS is a graphical Unix-like operating system for x86-64 computers.
...

out("{:c}", curr_value);
} else {
out("{:c}", curr_value);
if (!buffer_itter.is_end()) {
Copy link
Member

Choose a reason for hiding this comment

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

One issue that I can see here is that you're testing for the end of a relatively small buffer, not the end of the file. If the 32k buffer ends by \n, you will be missing a line count.

To fix that, I would take another approach:
Let's declare a struct that store the state, with the line count and a bool display_line_number.
The struct is passed to the function and also returned from it (as you did with line_count).
Then output_buffer_with_line_numbers can simply be:

for (auto const c : buffer) {
	if (display_line_number) {
		out("{: >6}\t", line_count);
		display_line_number = false;
	}
	if (c == '\n')
		display_line_number = true;
	out("{:c}", c);
}

Copy link
Author

Choose a reason for hiding this comment

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

Yea, sorry about the bad code I was rushing as I haven't had much time this week to work on projects. Will have another go at it on Sunday. Thanks for the help though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants