Closed
Bug 1088790
Opened 10 years ago
Closed 10 years ago
dosprint() doesn't support %zu and other size formats
Categories
(NSPR :: NSPR, enhancement, P1)
NSPR
NSPR
Tracking
(Not tracked)
RESOLVED
FIXED
4.10.9
People
(Reporter: mikeh, Assigned: mikeh)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
superreview+
wtc
:
checked-in+
|
Details | Diff | Splinter Review |
This makes it hard, e.g., to PRLog size_t types across 32- and 64-bit platforms.
PR_LOG( ..., ("foo=%zu", (size_t)foo));
...shows up in the output (on b2g) as:
PRLog: foo=%zu
Assignee | ||
Comment 1•10 years ago
|
||
I've tested this with --enable-32bit and -64bit on my PC (Ubuntu 14.04) and the test program passes. Is there an NSPR-version of the try-server I can bounce this off of?
Attachment #8511317 -
Flags: review?(ted)
Comment 2•10 years ago
|
||
Comment on attachment 8511317 [details] [diff] [review]
Add support for (s)size_t types (e.g. "%zu") to dosprintf(), v1
Review of attachment 8511317 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry for the review delay. Overall this looks good, I just have a few comments.
Also, no, sadly we do not have a NSPR try and we don't run NSPR tests as part of Firefox builds, so... You could probably write a hacky build system patch to forcibly run the NSPR tests in make check, I'm not sure we'd want to land that but it might serve to get you enough test coverage to be comfortable landing this.
::: pr/src/io/prprf.c
@@ +379,5 @@
> }
>
> /*
> ** BuildArgArray stands for Numbered Argument list Sprintf
> ** for example,
Can you nick the trailing whitespace while you're here?
@@ +523,5 @@
> + if (sizeof(size_t) == sizeof(PRInt32)) {
> + nas[ cn ].type = TYPE_INT32;
> + } else if (sizeof(size_t) == sizeof(PRInt64)) {
> + nas[ cn ].type = TYPE_INT64;
> + }
I realize other possible sizes are not likely to occur on any architecture that exists, but maybe an else { type = TYPE_UNKNOWN } would be a sane thing to add here?
@@ +547,5 @@
>
> case 'p':
> /* XXX should use cpp */
> if (sizeof(void *) == sizeof(PRInt32)) {
> + nas[ cn ].type = TYPE_UINT32;
This looks like a whitespace-only change?
@@ +820,5 @@
> type = TYPE_INT64;
> c = *fmt++;
> }
> + } else if (c == 'z') {
> + if (sizeof(void *) == sizeof(PRInt32)) {
Why is this sizeof(void*) but you're using sizeof(size_t) in the other function?
Attachment #8511317 -
Flags: review?(ted)
Assignee | ||
Comment 3•10 years ago
|
||
Incorporate review feedback.
Attachment #8511317 -
Attachment is obsolete: true
Attachment #8528566 -
Flags: review?(ted)
Assignee | ||
Comment 4•10 years ago
|
||
Fix a typo in a comment.
Attachment #8528566 -
Attachment is obsolete: true
Attachment #8528566 -
Flags: review?(ted)
Attachment #8528567 -
Flags: review?(ted)
Comment 5•10 years ago
|
||
Comment on attachment 8528567 [details] [diff] [review]
Add support for (s)size_t types (e.g. "%zu") to dosprintf(), v2.1
Review of attachment 8528567 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, I'll land this for you in a bit.
Attachment #8528567 -
Flags: review?(ted) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Hi Ted, just wondering what the time-frame is for landing this.
Flags: needinfo?(ted)
Comment 7•10 years ago
|
||
On Windows, PRI*SIZE are defined with "I" rather than "z".
From SizePrintfMacros.h:
#if defined(XP_WIN)
# define PRIoSIZE "Io"
[...]
I think it would be nice to recognize 'I' alongside 'z' so that these macros can be used.
Comment 8•10 years ago
|
||
Comment 9•10 years ago
|
||
Sorry, I totally forgot about this. Thanks for the reminder. I've landed your patch in the NSPR repo and tagged that revision as NSPR_4_10_9_BETA1. If you'd like to land this in m-c, file a bug for updating NSPR, then in your local clone run "client.py update_nspr NSPR_4_10_9_BETA1" to pull in the changes.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(ted)
Resolution: --- → FIXED
Updated•9 years ago
|
Severity: normal → enhancement
Priority: -- → P1
Target Milestone: --- → 4.10.9
Comment 10•9 years ago
|
||
Windows doesn't seem to have the signed ssize_t type, so the
prfz.c test doesn't compile on Windows.
The easiest fix seems to be assume that ssize_t is available
on Unix/POSIX platforms only and conditionally compile those
test cases only if the macro XP_UNIX is defined.
Attachment #8625358 -
Flags: superreview?(ted)
Attachment #8625358 -
Flags: review?(bugzilla)
Updated•9 years ago
|
Attachment #8625358 -
Flags: superreview?(ted) → superreview+
Comment 11•9 years ago
|
||
Comment on attachment 8625358 [details] [diff] [review]
Conditionally compile the ssize_t test cases for Unix only
https://hg.mozilla.org/projects/nspr/rev/9cd44a67ff5b
Attachment #8625358 -
Flags: checked-in+
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8625358 [details] [diff] [review]
Conditionally compile the ssize_t test cases for Unix only
Looks like this has already landed, so removing r?.
Attachment #8625358 -
Flags: review?(bugzilla)
You need to log in
before you can comment on or make changes to this bug.
Description
•