Skip to content

libsa: Add isprint()#1740

Closed
kpowkitty wants to merge 2 commits into
freebsd:mainfrom
kpowkitty:libsa_isprint
Closed

libsa: Add isprint()#1740
kpowkitty wants to merge 2 commits into
freebsd:mainfrom
kpowkitty:libsa_isprint

Conversation

@kpowkitty

Copy link
Copy Markdown
Contributor

Add isprint() alongside other isfoo() functions in libsa.
Motivations for change:

  1. Future ACPICA work in loader needs it.
  2. libsa/hexdump.c already uses it.

Signed-off-by: Kayla Powell (AKA Kat) kpowkitty@FreeBSD.org

@kpowkitty kpowkitty requested a review from bsdimp as a code owner June 27, 2025 00:10
@github-actions

github-actions Bot commented Jun 27, 2025

Copy link
Copy Markdown

Thank you for taking the time to contribute to FreeBSD!
All issues resolved.

@bsdimp

bsdimp commented Jun 27, 2025

Copy link
Copy Markdown
Member

Things look good.

I'm kinda on the fence about including the following:
"libsa/hexdump.c uses it raw, will be replaced in a separate commit."
in the first commit message. since you're going to immediately follow it up with a separate commit, I'm not sure that saying so and then having that commit be the next one adds value. If there were a rework that was coming some weeks or months in the future, it would make sense though. Can you remove it? Or do you have a reason I'm not seeing to include it?

So the rest looks great.

Comment thread stand/libsa/stand.h Outdated

static __inline int isprint(int c)
{
return ((c >= ' ') && (c <= '~'));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor nit. I know it's taught that it's a best practices to have the extra parens, all the other isfoo() functions don't use this style. It's a subtle part of our style guide that we try to match the existing style of the file. Now, it's a horrible mix of return foo; and return (foo);, but the latter is what style(9) proscribes, so that's another lesson too: when in doubt or it's ambiguous, favor style(9) when it offers an opinion on something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, that makes complete sense---I knew this and got too focused on following style(9) general fundamentals that I forgot that tidbit. Thank you for reminding me, and now I know that for certain moving forward. Fixed.

libsa is missing isprint().
Adding it with the other isfoo() functions.

Signed-off-by: Kayla Powell (AKA Kat) <kpowkitty@FreeBSD.org>
Signed-off-by: Kayla Powell (AKA Kat) <kpowkitty@FreeBSD.org>
@concussious

Copy link
Copy Markdown
Contributor

Does this need an update to libsa.3 (the manual page?). If so, I can help!

@bsdimp

bsdimp commented Jul 9, 2025

Copy link
Copy Markdown
Member

Does this need an update to libsa.3 (the manual page?). If so, I can help!

We do need it, but libsa.3 is also somewhat out of date and needs a lot of attention with someone that can read the code and see what's missing. It doesn't need to be with this commit, which I think is otherwise ready. Would you feel comfortable putting it into your list @concussious and looping me and @kpowkitty into the loop when you do? You can I can work on what's missing in libsa.4 as well, if you're wanting to really help a lot :).

@bsdimp bsdimp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK. I think this is ready to go. I'll land the change soon.

@bsdimp bsdimp added the merged Closed commit that's been merged label Jul 23, 2025
@bsdimp

bsdimp commented Jul 23, 2025

Copy link
Copy Markdown
Member

Automated message from ghpr: Thank you for your submission. This PR has been merged to FreeBSD's branch. These changes will appear shortly on our GitHub mirror.

@bsdimp bsdimp closed this Jul 23, 2025
freebsd-git pushed a commit that referenced this pull request Jul 23, 2025
libsa is missing isprint().  Adding it with the other isfoo()
functions. Remove a stray copy from fdt too.

Signed-off-by: Kayla Powell (AKA Kat) <kpowkitty@FreeBSD.org>
Reviewed by: imp
Pull Request: #1740
freebsd-git pushed a commit that referenced this pull request Jul 23, 2025
Signed-off-by: Kayla Powell (AKA Kat) <kpowkitty@FreeBSD.org>
Reviewed by: imp
Pull Request: #1740
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged Closed commit that's been merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants