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

Update to printf.3 based on OpenBSD manual page structure #1200

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chrisdavidson
Copy link
Contributor

Bug #247900
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=247900

printf.3
Update order of parameters alphabetically
Add parameter syntax to each parameter for clarity Untangle multiple opton entries into individual entries Update sentence structure throughout
Provide clarity on symbol based parameters

@chrisdavidson
Copy link
Contributor Author

This is a new pull request to address the questions/comments/suggestions as described in pull request: #1149

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed you changed the semantic Defined variable macro to the presentational Emphasis macro for stdout.

The semantic (regular) Variable macro (usually) renders the same way, how does everyone feel about using that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed you changed the semantic Defined variable macro to the presentational Emphasis macro for stdout.

The semantic (regular) Variable macro (usually) renders the same way, how does everyone feel about using that?

it looks good for stdout

Copy link
Member

Choose a reason for hiding this comment

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

We generally use Dv stdout although there are a few instances of Em and Va. I would prefer Dv.

@concussious
Copy link
Contributor

Just curious, what kind of tooling are you using that's putting g's at the end of lines?

@chrisdavidson
Copy link
Contributor Author

no tooling, it is my finger typing that is causing the headache and i am working improving my neovim interaction to resolve this.

.Fn asprintf
and
.Fn vasprintf
dynamically allocate a new string with
.Xr malloc 3 .
.Xr malloc 3
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the previous one, "dynamically allocate a new string with malloc(3)." seems better than the explicit mentioning of "that is stored in ret." (ret is the return pointer, but confusing here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see that and it does read better having the ret removed, updates will have this change.

Copy link
Member

Choose a reason for hiding this comment

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

We generally use Dv stdout although there are a few instances of Em and Va. I would prefer Dv.

@@ -297,13 +335,14 @@ should be grouped and separated by thousands using
the non-monetary separator returned by
.Xr localeconv 3 .
.El
.It
.Itg
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.Itg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved.

The
.Vt double
argument is rounded and converted in the style
argument is rounded and converted tog
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
argument is rounded and converted tog
argument is rounded and converted to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

The exponent always contains at least two digits; if the value is zero,
the exponent is 00.
where the number of digits after the hexadecimal-point character
is equal to theg
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
is equal to theg
is equal to the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

lib/libc/stdio/printf.3 Outdated Show resolved Hide resolved
respectively.
These conversion characters are deprecated, and will eventually disappear.
.It Cm eE
to represent theg
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to represent theg
to represent the

lib/libc/stdio/printf.3 Outdated Show resolved Hide resolved
.Li INFg
org
.Li -INF .
If the argument is not-a-number (NaN), it is converted tog
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If the argument is not-a-number (NaN), it is converted tog
If the argument is not-a-number (NaN), it is converted to

.It Cm B
The
.Vt unsigned int
(or apporiate variant) argument is converted to unsigned binary.
Copy link
Member

Choose a reason for hiding this comment

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

This is a little thin compared to, say, o or x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, would you recommend evaluating what o and x look like and see how it aligns with B since they are similar specifiers?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

.It Cm b
The
.Vt unsigned int
(or appropriate variant) argument is converted to unsigned binary.
Copy link
Member

Choose a reason for hiding this comment

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

This is a little thin compared to, say, o or x.

@@ -29,7 +29,10 @@
.\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
.\" SUCH DAMAGE.
.\"
.Dd September 5, 2023
.\" -
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.\" -

.Pp
.Cm A
conversion uses the prefix
.Dq Li 0X ,
Copy link
Contributor

Choose a reason for hiding this comment

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

The Ql macro is the preferred macro for formatting literal inline fragments. Historically, Dq Li was the preferred way before the deprecation of Li.
~style.mdoc(5)

With that change, would it appropriate to use Sq/Ql elsewhere as well for consistency? These render with single quotes on console, so it looks inconsistent from where where the file is previously using Dq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i am not sure i am following. The recommendation is to make all from .Sq/.Ql -> .Dq basee on your reference?

lib/libc/stdio/printf.3 Show resolved Hide resolved
@chrisdavidson
Copy link
Contributor Author

and now we have some more details with b and B options, these updates are based on the wording and structure of the x and X documentation, with the necessary conversation to binary notation.

Copy link
Member

@dag-erling dag-erling left a comment

Choose a reason for hiding this comment

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

I'm beginning to question how seriously you're taking this.

.Cm B
.Sm on
.Pp
The
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant now.

lib/libc/stdio/printf.3 Outdated Show resolved Hide resolved
lib/libc/stdio/printf.3 Outdated Show resolved Hide resolved
@chrisdavidson
Copy link
Contributor Author

Thank you all for the suggestions/comments/recommendations on this pull request.

The latest one, has incorporated these changes along with the refinement of b and B options. The options were updated based on the std documentation shared.

Please let me know if there is anything missing or refined.

Again thank you for the guidance on this manual page update.

@dag-erling
Copy link
Member

  1. There are still eight instances of “theg” and one instance of “tog” in the text. They are all individually marked in this review and trivial to find using your editor's search function, yet they are still present.
  2. You keep making up new incorrect text for the B and b conversion specifiers instead of just copying the text for X and x verbatim and changing only what needs to change.
  3. Please move the SPDX tag to the top like you were asked. It needs to be preceded and followed by exactly one blank comment line. There also needs to be a single blank comment line between the bottom of the disclaimer and the .Dd line.
  4. Please review your changes using git diff freebsd/main.. and man lib/libc/stdio/printf.3 before pushing.
  5. Please stop rebasing your branch, it makes it harder for us to review your progress. We will rebase and possibly squash as we see fit when (if) we commit your patch.

.Pp
Similar to
.Cm b
with the variation on the string value prefix
Copy link
Member

Choose a reason for hiding this comment

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

Ungrammatical.

Similar to
.Cm b
with the variation on the string value prefix
.Sq Ob
Copy link
Member

Choose a reason for hiding this comment

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

Really?

with the variation on the string value prefix
.Sq Ob
to
.Sq OB
Copy link
Member

Choose a reason for hiding this comment

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

Really?

lib/libc/stdio/printf.3 Show resolved Hide resolved
.Pp
For non zero results the prefix
.Sq 0b
will be prepended to result.
Copy link
Member

Choose a reason for hiding this comment

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

Really? Always?

lib/libc/stdio/printf.3 Show resolved Hide resolved
lib/libc/stdio/printf.3 Show resolved Hide resolved
.Ar u
except that the
.Ar unsigned int
argument is converted to unsigned octal notation.
Copy link
Member

Choose a reason for hiding this comment

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

Why does o now use different wording than b and x?

lib/libc/stdio/printf.3 Show resolved Hide resolved
lib/libc/stdio/printf.3 Show resolved Hide resolved
@bsdimp
Copy link
Member

bsdimp commented May 25, 2024

ping? what are the plans here? It can't land as it is, and the changes @dag-erling is suggesting are more intensive than I can do while prepping to land the change.

@chrisdavidson
Copy link
Contributor Author

i am currently working through @dag-erling recommendarions and requests. as it stands, it's going to be a little while.. is there a way to tag as "going slow but it's going"?

@bsdimp
Copy link
Member

bsdimp commented May 25, 2024

I'm unsure. I'll check back every couple of weeks or so. Take your time.

@bsdimp bsdimp added the man-only label Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants