Skip to content

Xsnprintf to stringutils#161

Merged
fasterit merged 4 commits intohtop-dev:masterfrom
BenBE:xsnprintf-to-stringutils
Oct 18, 2020
Merged

Xsnprintf to stringutils#161
fasterit merged 4 commits intohtop-dev:masterfrom
BenBE:xsnprintf-to-stringutils

Conversation

@BenBE
Copy link
Copy Markdown
Member

@BenBE BenBE commented Sep 19, 2020

This is a follow-up patch after #155 as asked for by @fasterit

I'd appreciate it if we could do this one after #155, #156 and #160.

@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch from 7b674ac to 825b7d7 Compare September 23, 2020 16:29
@BenBE BenBE marked this pull request as ready for review September 23, 2020 16:41
@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch 4 times, most recently from 008af64 to 1c026e2 Compare September 23, 2020 20:21
@ghost
Copy link
Copy Markdown

ghost commented Sep 23, 2020

Congratulations 🎉. DeepCode analyzed your code in 2.596 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch from 1c026e2 to 89695f7 Compare September 28, 2020 22:01
@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch from 89695f7 to 1e1dcf1 Compare September 29, 2020 16:09
@cgzones cgzones added the needs-rebase Pull request needs to be rebased and conflicts to be resolved label Oct 7, 2020
@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch 5 times, most recently from a01cca4 to 7bc1439 Compare October 12, 2020 20:54
@cgzones cgzones removed the needs-rebase Pull request needs to be rebased and conflicts to be resolved label Oct 13, 2020
@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch from 7bc1439 to 22da075 Compare October 13, 2020 21:53
@cgzones
Copy link
Copy Markdown
Member

cgzones commented Oct 14, 2020

Personally I do not like the names String_asprintf, String snprintf and String_strdup, because it might not be obvious what the differences to the standard functions are.
The x prefix for allocation wrappers is somewhat common to signal a checked or garbage-collected allocation, so xasprintf might be easier translated into checked asprintf.

But if @fasterit or @natoscott is fine with the naming, I'll go along.

@cgzones cgzones mentioned this pull request Oct 14, 2020
@fasterit
Copy link
Copy Markdown
Member

Personally I do not like the names String_asprintf, String snprintf and String_strdup, because it might not be obvious what the differences to the standard functions are.
The x prefix for allocation wrappers is somewhat common to signal a checked or garbage-collected allocation, so xasprintf might be easier translated into checked asprintf.

But if @fasterit or @natoscott is fine with the naming, I'll go along.

How about renaming StringUtils.{h|c} to XUtils.{h|c} and joining the remainder of XAlloc.{h|c} into this (i.e. XAlloc.{h|c} files (not functions) cease to exist)?
This way we could easily stay with naming functions xSnprintf and alike. That means this PR has much less noise where the functions are used, just cleaned and more logical includes.

@BenBE
Copy link
Copy Markdown
Member Author

BenBE commented Oct 14, 2020

I basically see 4 options to proceed here:

  1. Keep xSnprintf, xAsprintf and xStrdup in XAlloc.[ch] (AS IS)
  2. Move xSnprintf, xAsprintf and xStrdup to StringUtils.[ch], but keep current names (first patch only)
  3. Move xSnprintf, xAsprintf and xStrdup to StringUtils.[ch], rename for consistency (both patches)
  4. Join xSnprintf, xAsprintf, xStrdup and StringUtils.[ch] into new XUtils.[ch], but keep current names
  5. Join xSnprintf, xAsprintf, xStrdup and StringUtils.[ch] into new XUtils.[ch], and adjust functions String_* names for consistency
  6. Yet another option.

My personal favorite is variant 2.

FWIW: Currently #160 builds on this PR, but I can separate them quite easily if discussion here still needs more time and we want to unblock the include cleanup PR in the meantime. Rebasing the last two commits of #160 requires IWYU anyway, so makes no big difference which commit I rebase it onto.

@fasterit
Copy link
Copy Markdown
Member

4. Join `xSnprintf`, `xAsprintf`, `xStrdup` and `StringUtils.[ch]` into new `XUtils.[ch]`, but keep current names

My favorite.
/DLange

@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch from 22da075 to 41575c6 Compare October 14, 2020 23:51
@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch 2 times, most recently from 67d5a3f to 2b80473 Compare October 16, 2020 19:14
@BenBE
Copy link
Copy Markdown
Member Author

BenBE commented Oct 16, 2020

Could someone please check the deepcode issue? I don't understand what TF it want's to be done …

@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch from 2b80473 to d4b87cb Compare October 17, 2020 08:52
Copy link
Copy Markdown
Member

@cgzones cgzones left a comment

Choose a reason for hiding this comment

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

LGTM

@BenBE BenBE force-pushed the xsnprintf-to-stringutils branch from d4b87cb to c138d14 Compare October 17, 2020 18:56
@fasterit
Copy link
Copy Markdown
Member

Merged, thank you very much. Much appreciated cleanup work!

@fasterit fasterit merged commit c138d14 into htop-dev:master Oct 18, 2020
@fasterit fasterit added the hacktoberfest-accepted Accepted and eligible for Hacktoberfest label Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest-accepted Accepted and eligible for Hacktoberfest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants