Removed default '#' from %p#293
Closed
stefano-zanotti-88 wants to merge 1 commit into
Closed
Conversation
Owner
|
This is very insightful and IMO a nice improvement to the code size- if users really want %p to add the 0x, they can add the # themselves. I'm happy to merge this but the tests need some updating. I also think your other proposal in #288 is good so I think there's now an ordering issue. What order would you like to proceed in for these PRs? |
Contributor
Author
|
I'd go with #288 first. |
Owner
|
Implemented as #305 - thanks for the PR! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
%p is implementation-defined, so the current implementation is fine.
However, it introduces a default '#'. This is present in other implementations as well, though not all of them.
%p respects width and precision specifiers, as well as flags. [edit: currently, %p explicitly ignores the precision: out_spec->prec_opt = NPF_FMT_SPEC_OPT_NONE]
So, it seems inconsistent that it should not respect the presence/absence of '#'.
Before vs after this change:
You could argue that with this change, %p is mostly useless, as it is equivalent to %x with appropriate parameters; however, that's mostly true in any case, and the major benefit of %p is that you don't need a cast to the appropriate integer type (like casting to uintptr_t and using %tx, or casting to (unsigned long) and using %lx, or something else, depending on your platform).
Indeed, it could be easier just to change line 914 from:
to
Note that this, just like the current behavior, always adds "0x", whereas the "%x" equivalent proposed in this change omits it when the number is 0 (a very weird corner case required by the standard).
I'd say it could be more logical, and also result in smaller code, to apply the one-line change instead.
Also, this would allow for an easier removal of %p support, which would reduce the code footprint. I'm sure that not everybody has a need for %p.
Also note: if '#' is made optional here, and you accept #288 (making '#'-support optional for NPF), then further work would be needed to completely remove the 'need_0x' logic when '#'-support is disabled.