puts optional arguments and documentation#2
puts optional arguments and documentation#2chrisdedman merged 36 commits intosandbox-science:mainfrom
Conversation
chrisdedman
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
user/printf.c
Outdated
| #include <stdarg.h> | ||
| #include <stddef.h> | ||
|
|
||
| // TODO: |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
Added format string capabilities to the puts function in printf.c and included the documentation in doc/contents/puts.tex
Other changes: