Closed
Bug 1384233
Opened 8 years ago
Closed 8 years ago
Remove SizePrintfMacros.h and PRI*SIZE macros
Categories
(Core :: MFBT, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
Details
Attachments
(2 files)
As per https://groups.google.com/d/msg/mozilla.dev.platform/rbPn9KgS364/z-LZK1bgAgAJ we can drop these macros since VS 2015 (which is our minimum MSVC requirement) supports %z.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bugmail
Assignee | ||
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
I think you may want to also remove the %I support from mozglue/misc/Printf.cpp
Assignee | ||
Comment 3•8 years ago
|
||
The try push above had more failures than I like to see. They seem to all be pre-existing intermittents but to be safe I did another try push [1] rebased onto today's tip and with a second patch that does comment 2. It looks like the only places we use %I in our code now are in third-party libraries [2] which don't go through mozglue/misc/Printf.cpp so hopefully this is fine.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=39962faea5c40d91713cccf302f3e036397d3d9e
[2] http://searchfox.org/mozilla-central/search?q=print.*%25I&case=true®exp=true&path=
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
||
Comment 6•8 years ago
|
||
glandium said that %z wasn't listed in the MSDN docs for 2015, and AFAICT, he's correct. Have you tested and found otherwise?
Flags: needinfo?(bugmail)
Assignee | ||
Comment 7•8 years ago
|
||
Yeah, I tested locally and it seems to work. There's also some tests in the code [1] that ensure it works as expected. I also found some comments online [2] that seem to indicate support was added.
[1] https://hg.mozilla.org/try/rev/1985b0578b3a51b12c6945b680b21ed888aec746#l117.1
[2] https://blogs.msdn.microsoft.com/vcblog/2014/06/03/visual-studio-14-ctp/#div-comment-77743 (the previous comment specifically asks about %zu)
Flags: needinfo?(bugmail)
Comment 8•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> It looks like
> the only places we use %I in our code now are in third-party libraries [2]
> which don't go through mozglue/misc/Printf.cpp so hopefully this is fine.
I'm having some second thoughts about removing the printf support.
It's not too uncommon for third-party libraries in-tree to be hacked to
redirect their logging to MOZ_LOG. So perhaps leaving the %I support in
place would let us avoid a problem or two.
![]() |
||
Comment 9•8 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> Yeah, I tested locally and it seems to work. There's also some tests in the
> code [1] that ensure it works as expected. I also found some comments online
> [2] that seem to indicate support was added.
For avoidance of doubt, does "I tested locally" mean something like "I used MSVC 2015 to compile a program printing with %zu and said program worked as expected"?
> [1]
> https://hg.mozilla.org/try/rev/1985b0578b3a51b12c6945b680b21ed888aec746#l117.1
This test is for our own implementation of printf (don't ask), not the system printf.
> [2]
> https://blogs.msdn.microsoft.com/vcblog/2014/06/03/visual-studio-14-ctp/#div-
> comment-77743 (the previous comment specifically asks about %zu)
I would prefer official docs, but this is better than nothing, I guess.
Comment 10•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #9)
> > [2]
> > https://blogs.msdn.microsoft.com/vcblog/2014/06/03/visual-studio-14-ctp/#div-
> > comment-77743 (the previous comment specifically asks about %zu)
>
> I would prefer official docs, but this is better than nothing, I guess.
Not quite official docs, but this blog post [1] pretty clearly states:
"""
The following length modifiers are now supported:
hh: signed char or unsigned char
j: intmax_t or uintmax_t
t: ptrdiff_t
z: size_t
L: long double
"""
[1] https://blogs.msdn.microsoft.com/vcblog/2014/06/18/c-runtime-crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #9)
> For avoidance of doubt, does "I tested locally" mean something like "I used
> MSVC 2015 to compile a program printing with %zu and said program worked as
> expected"?
Yeah, I added some printf calls in firefox (local windows build using MSVC 2015) to test the %zu %zo %zx and %zX format specifiers and they all produced the correct output.
> This test is for our own implementation of printf (don't ask), not the
> system printf.
Ah, good point.
I'm happy to retract the second patch based on Tom's comment 8. It does seem unnecessarily risky.
Comment 12•8 years ago
|
||
It's also worth considering bug 1331349. My questions from that experience are
(1) whether this works ok on mingw, and (2) whether gcc emits errors on windows
when %z is used.
![]() |
||
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8890466 [details]
Bug 1384233 - Remove SizePrintfMacros.h.
https://reviewboard.mozilla.org/r/161602/#review166952
Thank you!
::: commit-message-e8400:4
(Diff revision 1)
> +Bug 1384233 - Remove SizePrintfMacros.h. r?froydnj
> +
> +We have a minimum requirement of VS 2015 for Windows builds, which supports
> +the %zu family of format specifiers. So we don't need SizePrintfMacros.h
Nit: I think it'd be more correct to say "the z length modifier for format specifiers"
::: dom/media/MediaDecoderStateMachine.cpp:3322
(Diff revision 1)
> {
> MOZ_ASSERT(OnTaskQueue());
> MOZ_ASSERT(IsVideoDecoding());
> MOZ_ASSERT(!IsRequestingVideoData());
> MOZ_ASSERT(!IsWaitingVideoData());
> - LOGV("Queueing video task - queued=%" PRIuSIZE ", decoder-queued=%" PRIoSIZE
> + LOGV("Queueing video task - queued=%zu, decoder-queued=%zo"
`%zo`? I know that's what the original code had, but still, weird.
Attachment #8890466 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8890467 [details]
Bug 1384233 - Update documentation for the %I format specifier in Printf.h.
https://reviewboard.mozilla.org/r/161604/#review166958
I think Tom is right; we should keep the format specifier, but we should add a comment in `Printf.h` about why we're keeping it...and also remove the `PRI*SIZE` bit from that comment. Can you do that, please?
Attachment #8890467 -
Flags: review?(nfroyd)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
||
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8890467 [details]
Bug 1384233 - Update documentation for the %I format specifier in Printf.h.
https://reviewboard.mozilla.org/r/161604/#review166966
Thank you! r=me with optional suggestion below.
::: mozglue/misc/Printf.h:42
(Diff revision 2)
> +** users should prefer using %z instead of %I. We are leaving in the
> +** %I support in case third-party code that uses it gets routed to
Maybe "We are support %I in addition to %z..." instead of "We are leaving in %I support..."? And then say "...third-party code that uses %I gets routed..." to be extra clear.
Attachment #8890467 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #17)
> Maybe "We are support %I in addition to %z..." instead of "We are leaving in
> %I support..."? And then say "...third-party code that uses %I gets
> routed..." to be extra clear.
That sounds better, thanks! I'll update the patch and land shortly.
Comment hidden (mozreview-request) |
Comment 20•8 years ago
|
||
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd104d6d9adf
Remove SizePrintfMacros.h. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/6c5ab3bb451d
Update documentation for the %I format specifier in Printf.h. r=froydnj
Comment 21•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd104d6d9adf
https://hg.mozilla.org/mozilla-central/rev/6c5ab3bb451d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
![]() |
||
Comment 22•8 years ago
|
||
Kartikaya, thanks for cc'ing me on this bug.
You need to log in
before you can comment on or make changes to this bug.
Description
•