Closed Bug 1384233 Opened 6 years ago Closed 6 years ago

Remove SizePrintfMacros.h and PRI*SIZE macros

Categories

(Core :: MFBT, enhancement)

Product:
Core
Component:
MFBT
Version:
Other Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(2 files)

Bug 1384233 - Remove SizePrintfMacros.h.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1384233 - Update documentation for the %I format specifier in Printf.h.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
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: nobody → bugmail
I think you may want to also remove the %I support from mozglue/misc/Printf.cpp
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&regexp=true&path=
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)
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)
(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.
(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.
(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/
(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.
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 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 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 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+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/dd104d6d9adf
https://hg.mozilla.org/mozilla-central/rev/6c5ab3bb451d
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.