-
Notifications
You must be signed in to change notification settings - Fork 2
puts optional arguments and documentation #2
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
Conversation
chrisdedman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution. I have a couple of questions that I left in the code review before I can merge it. Otherwise, I think it is pretty good.
Let's talk about my comments :)
| } | ||
|
|
||
| unsigned long long _bdiv(unsigned long long dividend, unsigned long long divisor, unsigned long long *remainder) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to add a safeguard in case the divisor is zero (division by zero would crash the system).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done in the future, along with a faster division algorithm. The current use I've put this algorithm through involves dividing only by 10 and 16 so it isn't an issue right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. I will be adding a comment in the code to specify this (just in case we forget moving forward).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing about this for the future. While thinking about implementing division, I found out that Linux implements its own 64 bit by 64 bit division algorithm in linux/math64.h(I believe). You can reference that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user/printf.c
Outdated
| #include <stdarg.h> | ||
| #include <stddef.h> | ||
|
|
||
| // TODO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should be the behavior of those cases, for example:
puts("%d %d\n", 10); // Fewer arguments than specifiers
puts("%d\n", 10, 20, 30); // More arguments than specifiersor maybe this:
puts("Lone percent at end -> %\n"); // currently print: Lone percent at end ->
puts("Unknown specifier -> %z\n"); // currently print: Unknown specifier ->
puts("%s\n", NULL); // currently print an empty charThose are the few edge cases I think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
puts("%d %d\n", 10); // Fewer arguments than specifiers
puts("%d\n", 10, 20, 30); // More arguments than specifiersThe current behaviour for the first case is the same as glibc printf, which is reading random garbage values from the stack and printing it out, when a matching argument is not specified.
In the second case, only 10 will be printed, and the rest will be discarded without any fanfare.
puts("Lone percent at end -> %\n"); // currently print: Lone percent at end ->
puts("Unknown specifier -> %z\n"); // currently print: Unknown specifier ->
puts("%s\n", NULL); // currently print an empty charThe first case and the second case will be in the same category, as the \n will be grouped with the %. Even if you leave a space, it will be grouped with the %. If instead, such a case were given
puts("Percent: %");In glibc printf, the above line returns with -1 to indicate an error, and also does not print the line. I do not think this is necessary, because the programmer would know something went wrong if their purpose was to:
- Print a
% - Print some other format specifier.
from the awkward output. Also, a partially printed line would probably be better for debugging than a line that is not printed at all, since you can at least pin point where it went wrong.
I don't think the second case will be much of a problem. I intended unused and unrecognized format specifiers to be skipped, and if one desires to print something like z% of Y they can use the format %%z of Y.
The third case is an oversight on my part, as it allows null pointer dereferencing. Would it be adequate if the string (null) were printed if a NULL pointer were encountered?
If there are any other changes that you'd like, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the puts("Lone percent at end -> %\n"); and puts("Unknown specifier -> %z\n"); in C implementation, you will get a warning, and the percent printed (see screenshot from GDB). Not sure if we should allow the same behavior. It could be worth checking the behavior in other kernels.

For the NULL, in the C implementation, it prints just NULL NULL -> (null). So, I think (like you mentioned) maybe follow the same behavior string (null) for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing about the warnings in this context I think that it will just complicate the implementation without much payoff, as I've mentioned earlier. But if you find similar models in other kernels too, then let me know.
| } | ||
|
|
||
| va_end(elem_list); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we return the length of the string like the C implementation of printf() for debugging purposes?
https://cplusplus.com/reference/cstdio/printf/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see this being useful for, taking an example, if execution stops with an error for some case in your edge cases comment. Will there be any other use for this?
I suppose that then an error handling routine would also be appropriate, like the set errno and perror model used in the Linux API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't know what other use case we could have, but it could be a good idea to have the option.
I was thinking about having some error handling anyway, so maybe we can look up the implementation in the Linux API indeed.
chrisdedman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Added format string capabilities to the puts function in printf.c and included the documentation in doc/contents/puts.tex
Other changes: