1197328 - remove PR_snprintf calls in media/{webrtc,mtransport}/

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P5)

Description

[Nathan Froyd [:froydnj]]

Steps:

1. Find all calls to PR_snprintf in media/webrtc/ and media/mtransport/.
2. Add #include "mozilla/Snprintf.h" to the file(s).
3. Replace PR_snprintf(X, sizeof(X), ...) calls with snprintf_literal(X, ...).
4. Replace other PR_snprintf calls with snprintf.
5. Remove any #include "prprf.h" lines.

Note that the bits in media/libstagefright/ are explicitly not included in this bug.

Comment 1

[Eric Rescorla (:ekr)]

Can you explain the reason for this change? We depend extensively on NSPR in
any case.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #1)
> Can you explain the reason for this change? We depend extensively on NSPR in
> any case.

Bug 1197205 comment 0 has some rationale.  Using things that are in the standard, when possible, is better than inventing our own.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: convert PR_snprintf to snprintf_literal (media/webrtc)

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8651382 [details] [diff] [review]
convert PR_snprintf to snprintf_literal (media/webrtc)

Review of attachment 8651382 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +702,5 @@
>                (strncmp(HELLO_INITIATOR_URL_START, locationCStr.get(),
>                         strlen(HELLO_INITIATOR_URL_START)) == 0);
>    }
>  
> +  snprintf_literal(

I'm a little confused about the status of this function. Is it in standard C or just a Mozilla shim?

@@ +710,5 @@
>        static_cast<unsigned long long>(mWindow ? mWindow->WindowID() : 0),
>        locationCStr.get() ? locationCStr.get() : "NULL");
>  
>  #else
> +  snprintf_literal(temp, "%llu", (unsigned long long)timestamp);

If you're going to change this code, please fix the casts to be c++ style.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla (:ekr) from comment #4)

> >  
> > +  snprintf_literal(
> 
> I'm a little confused about the status of this function. Is it in standard C
> or just a Mozilla shim?

A (simple) shim; see mfbt/Snprintf.h

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #5)
> (In reply to Eric Rescorla (:ekr) from comment #4)
> 
> > >  
> > > +  snprintf_literal(
> > 
> > I'm a little confused about the status of this function. Is it in standard C
> > or just a Mozilla shim?
> 
> A (simple) shim; see mfbt/Snprintf.h

That seems like it makes this code more opaque, since it has a name
that implies it's a stdlib function.

Comment 7

[Joshpal Sahota]

Hi, my name is Joshpal Sahota. I am currently studying the Open-Source Development module at Coventry University and would like to be assigned to this 'simple first bug'.

Regards
Joshpal Sahota

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Joshpal Sahota from comment #7)
> Hi, my name is Joshpal Sahota. I am currently studying the Open-Source
> Development module at Coventry University and would like to be assigned to
> this 'simple first bug'.

I'm afraid this specific bug isn't good, as there's already a patch for it, and the only reason it hasn't landed isn't really the patch itself.

Comment 9

[chaithanya]

Attachment: Changes have been made as per the requirements.I can make the Modifications if there are any

Comment 10

[Eric Rescorla (:ekr)]

This doesn't address my concern: having a function which is not a stdlib function named like this
is massively confusing. Nathan, can we please rename this function somehow so it's clear it's a Mozilla-local thing and not part of the standard library.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #10)
> This doesn't address my concern: having a function which is not a stdlib
> function named like this
> is massively confusing. Nathan, can we please rename this function somehow
> so it's clear it's a Mozilla-local thing and not part of the standard
> library.

I'm open to renaming it, but I am having a hard time coming up with alternatives, especially since I'm unclear on what the objectionable part is  Is it that it includes the substring "snprintf" or that it starts with "snprintf" or something else?  I think renaming it to something else that removes the {sn,}printf part would make it even more opaque (format how?  why are we using this instead of snprintf?  etc.).

Comment 12

[Eric Rescorla (:ekr)]

What's objectionable is that it has a similar name to a library call but isn't one.
In general, I would prefer to never have unqualified/unprefixed all-lower-case
names like this in C code, but that seems especially true here.

moz_sprintf_literal() seems like it would be appropriate.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #12)
> What's objectionable is that it has a similar name to a library call but
> isn't one.
> In general, I would prefer to never have unqualified/unprefixed
> all-lower-case
> names like this in C code, but that seems especially true here.
> 
> moz_sprintf_literal() seems like it would be appropriate.

The point about all-lower-case names is a good one.  Seeing as how snprintf_literal is C++-only code, it would be somewhat unusual to have a prefixed name here.  Would you have the same objections to something like SprintfLiteral?

Comment 14

[Eric Rescorla (:ekr)]

That would be fine.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #14)
> That would be fine.

Great.  Since the renaming is outside the scope of this bug, can we go ahead and land this, and I'll follow through with the renaming shortly thereafter?

Comment 16

[Eric Rescorla (:ekr)]

Why don't you instead rename it first and then land this, thus removing the
need to change this code twice.

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8688011 [details] [diff] [review]
Changes have been made as per the requirements.I can make the Modifications if there are any

Review of attachment 8688011 [details] [diff] [review]:
-----------------------------------------------------------------

Heading in the right direction, but needs a few tweaks.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +11,5 @@
>  #include "MediaStreamGraphImpl.h"
>  #endif
>  
>  #include <math.h>
> +#include "mozilla/snprintf.h"

This should be mozilla/Snprintf.h.

@@ +490,1 @@
>                  inner_data[0],

This argument (and other arguments) need to be indented to line up with the |tmp| argument.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +705,1 @@
>        "%llu (id=%llu url=%s)",

These %llu format strings should be "%" PRIu64 instead.

@@ +707,5 @@
>        static_cast<unsigned long long>(mWindow ? mWindow->WindowID() : 0),
>        locationCStr.get() ? locationCStr.get() : "NULL");
>  
>  #else
> +  snprintf_literal(temp, "%llu", (unsigned long long)timestamp);

Likewise here.

Comment 18

[chaithanya]

Attachment: Changes have been made as per comment 17. In case of any more modifications please let me know

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8689169 [details] [diff] [review]
Changes have been made as per comment 17. In case of any more modifications please let me know

Review of attachment 8689169 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like this patch is missing some changes.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +11,5 @@
>  #include "MediaStreamGraphImpl.h"
>  #endif
>  
>  #include <math.h>
> +#include "mozilla/snprintf.h"

This should be "mozilla/Snprintf.h", as comment 17 requested.

@@ +485,5 @@
>                                                 len, len, &out_len);
>    if (!NS_SUCCEEDED(res)) {
>      char tmp[16];
>  
> +    snprintf_literal(tmp, "%.2x %.2x %.2x %.2x",

This patch is corrupt, it looks like?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +707,5 @@
>        static_cast<unsigned long long>(mWindow ? mWindow->WindowID() : 0),
>        locationCStr.get() ? locationCStr.get() : "NULL");
>  
>  #else
> +  snprintf_literal(temp, "%", (unsigned long long)timestamp);

This appears to have been incorrectly translated.

Comment 20

[chaithanya]

(In reply to Nathan Froyd [:froydnj] from comment #19)
> Comment on attachment 8689169 [details] [diff] [review]
> Changes have been made as per comment 17. In case of any more modifications
> please let me know
> 
> Review of attachment 8689169 [details] [diff] [review]:
> -----------------------------------------------------------------
> 

> >  #else
> > +  snprintf_literal(temp, "%", (unsigned long long)timestamp);
> 
> This appears to have been incorrectly translated.

The %llu format strings should be "%" PRIu64 right?  didn't understand what the string format must be changed to. Could you please make it clear?

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to chaithanya from comment #20)
> > >  #else
> > > +  snprintf_literal(temp, "%", (unsigned long long)timestamp);
> > 
> > This appears to have been incorrectly translated.
> 
> The %llu format strings should be "%" PRIu64 right?  didn't understand what
> the string format must be changed to. Could you please make it clear?

I don't understand the question here.  Before, the format string was "%llu".  Your patch makes it "%", which is incorrect.  You should make it "%" PRIu64, which uses the string concatenation feature of the C preprocessor to yield a format string that's correct on a variety of architectures.

Comment 22

[chaithanya]

Attachment: Hope I made the necesssary changes as by comment21. Please let me know if there are any more changes to make

Comment 23

[Eric Rescorla (:ekr)]

I'm not sure where this got lost, but per c16, please rename the function calls *first* so they aren't named in a confusing way and then change this file.

Comment 24

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #23)
> I'm not sure where this got lost, but per c16, please rename the function
> calls *first* so they aren't named in a confusing way and then change this
> file.

There's no sense in asking somebody to write code that won't even compile without patches that haven't been written yet.  I will take care of doing any necessary renamings prior to this code landing.

Comment 25

[Eric Rescorla (:ekr)]

(In reply to Nathan Froyd [:froydnj] (high latency through 29 November) from comment #24)
> (In reply to Eric Rescorla (:ekr) from comment #23)
> > I'm not sure where this got lost, but per c16, please rename the function
> > calls *first* so they aren't named in a confusing way and then change this
> > file.
> 
> There's no sense in asking somebody to write code that won't even compile
> without patches that haven't been written yet.

Sure. I'm not saying that that has to be done, merely that this code shouldn't
land without those changes.


>  I will take care of doing
> any necessary renamings prior to this code landing.

This is also fine.

Comment 26

[Nathan Froyd [:froydnj]]

Comment on attachment 8692565 [details] [diff] [review]
Hope I made the necesssary changes as by comment21. Please let me know if there are any more changes to make

Review of attachment 8692565 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't look like it can be applied unchanged, for the reasons detailed below.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +485,5 @@
>                                                 len, len, &out_len);
>    if (!NS_SUCCEEDED(res)) {
>      char tmp[16];
>  
> +    snprintf_literal(tmp, "%.2x %.2x %.2x %.2x",

Bugzilla's patch review interface doesn't want to display this hunk of your patch, which means that it's corrupt it some way.  Please make sure you're generating the patch via |hg diff| or |hg export| and not attempting to hand-edit the patch prior to posting.  In this particular case, the patch file has the line just below this starting with two tabs, which renders the patch file invalid.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ -706,1 @@
>        ""%" PRIu64 (id="%" PRIu64 url=%s)",

It looks like this original line doesn't appear in mozilla-central's copy of the code.  Please make sure your diff is generated against mozilla-central, and not previous versions of your patches.

Comment 27

[chaithanya]

Attachment: Made changes as by comment 26. Please let me know if any further changes are to be made.

Comment 28

[Nathan Froyd [:froydnj]]

Comment on attachment 8704247 [details] [diff] [review]
Made changes as by comment 26. Please let me know if any further changes are to be  made.

Review of attachment 8704247 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing things up!  The patch is readable now.  Minor changes below.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +495,5 @@
> +    snprintf_literal(tmp,"%.2x %.2x %.2x %.2x",
> +             inner_data[0],
> +             inner_data[1],
> +             inner_data[2],
> +             inner_data[3]);

The indentation changes requested in comment 17 haven't been done yet.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +707,2 @@
>        temp,
> +       "%" PRIu64 (id= "%" PRIu64 url=%s)",

This doesn't look like the correct syntax.

@@ +709,2 @@
>        static_cast<unsigned long long>(timestamp),
>        static_cast<unsigned long long>(mWindow ? mWindow->WindowID() : 0),

These casts should now be:

  static_cast<uint64_t>(...)

to make the types for the format string and the types for the arguments match.

@@ +710,5 @@
>        static_cast<unsigned long long>(mWindow ? mWindow->WindowID() : 0),
>        locationCStr.get() ? locationCStr.get() : "NULL");
>  
>  #else
> +  snprintf_literal(temp, "%" PRIu64, (unsigned long long)timestamp);

Likewise, this cast should be to uint64_t.

Comment 29

[chaithanya]

Attachment: Hope I have made necessary changes as by comment 28.

Comment 30

[Nathan Froyd [:froydnj]]

Comment on attachment 8704730 [details] [diff] [review]
Hope I have made necessary changes as by comment 28.

Review of attachment 8704730 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing up the casts and the indentation!  Unfortunately, there is some format string confusion now.  Please see below.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +707,2 @@
>        temp,
> +       ""%" PRIu64 (id= "%" PRIu64 url=%s)",

This line is still incorrect.  You have (inserting some spaces):

"" % " PRIu64 (id= " % " PRIu64 url=%s)"

So literal strings of:

""
" PRIu64 (id= "
" PRIu64 url=%s)"

The PRIu64 identifiers should go outside the strings (as they are macros) and the '%' characters should be inside strings to act as format characters.

@@ +711,4 @@
>        locationCStr.get() ? locationCStr.get() : "NULL");
>  
>  #else
> +  snprintf_literal(temp, "%PRIu64", (uint64_t)timestamp);

You changed the cast correctly, but you also changed the format string, and the format string is now wrong.  Please change it back to what you had in the previous version of the patch.

Comment 31

[chaithanya]

Attachment: made necessary changes as by comment 30. Please let me know if there are any further changes to be made.

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 8706068 [details] [diff] [review]
made necessary changes as by comment 30. Please let me know if there are any further changes to be made.

Review of attachment 8706068 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +707,2 @@
>        temp,
> +       "%"PRIu64 "%"(id= "%"PRIu64 url=%s)",

This is still not correct; notice how the |(id= | bit is outside quotes, as is the |url=%s)| bit, and you have an unmatched " character at the end.

You also need a space between strings and the PRIu64 macro due to C++ parsing rules.

The compiler will tell you about these sorts of errors; please use that as a first pass.

@@ +711,4 @@
>        locationCStr.get() ? locationCStr.get() : "NULL");
>  
>  #else
> +  snprintf_literal(temp, "%"PRIu64, (uint64_t)timestamp);

Likewise, you need to write "%" PRIu64 here (notice the space).

Comment 33

[chaithanya]

Attachment: Made changes as by comment 32.The patch has been tested locally.Please review.

Comment 34

[Nathan Froyd [:froydnj]]

Comment on attachment 8709145 [details] [diff] [review]
Made changes as by comment 32.The patch has been tested locally.Please review.

Review of attachment 8709145 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!  Just two small problems left.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +11,5 @@
>  #include "MediaStreamGraphImpl.h"
>  #endif
>  
>  #include <math.h>
> +#include "mozilla/Snprintf.h"

Your patch has this change, but not any other changes that previous patches have made to this file.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +707,2 @@
>        temp,
> +        "%" PRIu64 "%(id= %" PRIu64 "url=%s)",

This line should be indented to line up with the line above and below it.

Comment 35

[chaithanya]

Attachment: Made changes as by comment 34. Please review

Comment 36

[Nathan Froyd [:froydnj]]

Comment on attachment 8711404 [details] [diff] [review]
Made changes as by comment 34. Please review

Review of attachment 8711404 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the fixups.  Just a few last things.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +494,5 @@
> +    snprintf_literal(tmp, "%.2x %.2x %.2x %.2x",
> +		      inner_data[0],
> +		      inner_data[1],
> +		      inner_data[2],
> +		      inner_data[3]);

Please indent these inner_data lines to line up below |tmp|.  It looks like you have used some tabs for indentation, which might be causing things to look OK in your editor; please use spaces instead when modifying Mozilla code.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +706,2 @@
>        temp,
> +      "%" PRIu64 "%(id= %" PRIu64 "url=%s)",

This format string is lacking some spaces that the previous one had, and the "%(" is incorrect (apologies for not catching this error last time).

Comment 37

[chaithanya]

Attachment: Made changes as per comment 36. Please review.

Comment 38

[Nathan Froyd [:froydnj]]

Comment on attachment 8713986 [details] [diff] [review]
Made changes as per comment 36. Please review.

Review of attachment 8713986 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the cleanups; just two more minor fixups to go.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +716,2 @@
>        temp,
> +      "%" PRIu64 "% (id= %" PRIu64 "url=%s)",

The "% " starting off the string is incorrect; it should just be a single space.  There's also a space missing prior to the url= part, as noted in comment 36.

Comment 39

[chaithanya]

Attachment: Made changes as by comment 38. Please review.

Comment 40

[Nathan Froyd [:froydnj]]

Comment on attachment 8714878 [details] [diff] [review]
Made changes as by comment 38. Please review.

Review of attachment 8714878 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, one last fixup.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +716,2 @@
>        temp,
> +      "%" PRIu64 " (id= %" PRIu64 "url =%s)",

This change puts the space on the wrong side of |url|; it should be before, not after: " url=%s"

Comment 41

[chaithanya]

(In reply to Nathan Froyd [:froydnj] from comment #40)
> Comment on attachment 8714878 [details] [diff] [review]
> Made changes as by comment 38. Please review.
> 
> Review of attachment 8714878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good, one last fixup.
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +716,2 @@
> >        temp,
> > +      "%" PRIu64 " (id= %" PRIu64 "url =%s)",
> 
> This change puts the space on the wrong side of |url|; it should be before,
> not after: " url=%s"

Hi
  Thanks for that review. I had some issues with my PC. I will be submitting the patch soon.

Comment 42

[chaithanya]

Attachment: Apologies for the delay. Made changes as per comment 40 . Please review.

Comment 43

[Nathan Froyd [:froydnj]]

Comment on attachment 8726239 [details] [diff] [review]
Apologies for the delay. Made changes as per comment 40 . Please review.

Review of attachment 8726239 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thank you!

Comment 44

[chaithanya]

(In reply to Nathan Froyd [:froydnj] from comment #43)
> Comment on attachment 8726239 [details] [diff] [review]
> Apologies for the delay. Made changes as per comment 40 . Please review.
> 
> Review of attachment 8726239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, thank you!

Can we get this landed?

Comment 45

[Mark Capella [:capella]]

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=060e5dccf23644f8b52ed6802edf64356d0f50e1

Note rebased .diff to get it to apply, no logic changes.

Comment 46

[Nils Ohlmeier [:drno]]

(In reply to Mark Capella [:capella] from comment #45)
> Pushed to try
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=060e5dccf23644f8b52ed6802edf64356d0f50e1

That did not compile at all on any platform. Anyone still willing to fix this to move it forward?

Comment 47

[Igor]

I am highly willing to work on this. Can I produce another patch and post here?

Comment 48

[Nils Ohlmeier [:drno]]

(In reply to Igor from comment #47)
> I am highly willing to work on this. Can I produce another patch and post
> here?

Yes please :-)

Comment 49

[Igor]

Attachment: bug-1197328.patch

This is my first bug, please let me know if there is anything wrong with it.
PS: I am planning to dedicate a lot of my time to the project, so any mentoring is more than welcome.

Comment 50

[Nils Ohlmeier [:drno]]

(In reply to Igor from comment #49)
> Created attachment 8776779 [details] [diff] [review]
> bug-1197328.patch
> 
> This is my first bug, please let me know if there is anything wrong with it.
> PS: I am planning to dedicate a lot of my time to the project, so any
> mentoring is more than welcome.

I'm getting a compiler error with your patch:

13:18.98 make[5]: Entering directory `/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin15.6.0/dom/workers/test/gtest'
13:19.34 /Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:656:30: error: expected ')'
13:19.35   snprintf_literal(temp, "%" PRIu64, static_cast<uint64_t>(timestamp));
13:19.35                              ^
13:19.35 /Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:656:19: note: to match this '('
13:19.35   snprintf_literal(temp, "%" PRIu64, static_cast<uint64_t>(timestamp));
13:19.62                   ^
13:19.62 1 error generated.
13:19.62

Comment 51

[Igor]

(In reply to Nils Ohlmeier [:drno] from comment #50)
> (In reply to Igor from comment #49)
> > Created attachment 8776779 [details] [diff] [review]
> > bug-1197328.patch
> > 
> > This is my first bug, please let me know if there is anything wrong with it.
> > PS: I am planning to dedicate a lot of my time to the project, so any
> > mentoring is more than welcome.
> 
> I'm getting a compiler error with your patch:
> 
> 13:18.98 make[5]: Entering directory
> `/Users/nohlmeier/src/mozilla-central/obj-x86_64-apple-darwin15.6.0/dom/
> workers/test/gtest'
> 13:19.34
> /Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/
> peerconnection/PeerConnectionImpl.cpp:656:30: error: expected ')'
> 13:19.35   snprintf_literal(temp, "%" PRIu64,
> static_cast<uint64_t>(timestamp));
> 13:19.35                              ^
> 13:19.35
> /Users/nohlmeier/src/mozilla-central/media/webrtc/signaling/src/
> peerconnection/PeerConnectionImpl.cpp:656:19: note: to match this '('
> 13:19.35   snprintf_literal(temp, "%" PRIu64,
> static_cast<uint64_t>(timestamp));
> 13:19.62                   ^
> 13:19.62 1 error generated.
> 13:19.62

Probably __STDC_FORMAT_MACROS remains undefined in your architecture. What is the less intrusive approach here: change back the use of PRIu64 or try to define the macro in the proper place?

Comment 52

[Igor]

Attachment: bug-1197328.patch - with #include IntegerPrintfMacros.h

As a workaround, I followed what a file in the same tree (media-conduit/WebrtcOMXH264VideoCodec.cpp) does - include "mozilla/IntegerPrintfMacros.h". This probably will expose PRIu64 for use in PeerConnectionImpl.cpp.

drno, can you please confirm if the build error on mac persists?

Comment 53

[Eric Rescorla (:ekr)]

Note the discussion in c14 and prior about naming. When are the patches that remove the stdlib-style names landing?

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

Nathan - per comment 24 and comment 53 - are the patches to rename ready, or can you create a blocker bug for this one for the renaming?

Comment 55

[Nathan Froyd [:froydnj]]

(In reply to Randell Jesup [:jesup] from comment #54)
> Nathan - per comment 24 and comment 53 - are the patches to rename ready, or
> can you create a blocker bug for this one for the renaming?

I can create a blocker bug, though I honestly can't understand why "number of times we change a file" is a good rationale for doing things in this order.

Comment 56

[Eric Rescorla (:ekr)]

That's not the rationale. The rationale is that I don't want the code in this module to be more confusing than necessary and the current function names are confusing. And given that this bug is almost a year old and the rename still hasn't happened, I think it's quite appropriate for me to be concerned that these patches not land on some "eventually we'll rename" basis.

Comment 57

[Nathan Froyd [:froydnj]]

Comment on attachment 8776779 [details] [diff] [review]
bug-1197328.patch

Canceling review since a newer patch has been uploaded.

Comment 58

[Nathan Froyd [:froydnj]]

(In reply to Eric Rescorla (:ekr) from comment #56)
> That's not the rationale.

I will just cite:

(In reply to Eric Rescorla (:ekr) from comment #16)
> Why don't you instead rename it first and then land this, thus removing the
> need to change this code twice.

(In reply to Eric Rescorla (:ekr) from comment #56)
> And given that this bug is almost a year old and the
> rename still hasn't happened, I think it's quite appropriate for me to be
> concerned that these patches not land on some "eventually we'll rename"
> basis.

There have been other patches in-progress to rename in other places, and it didn't seem worth disrupting those in the meantime.

Comment 59

[Eric Rescorla (:ekr)]

(In reply to Nathan Froyd [:froydnj] from comment #58)
> (In reply to Eric Rescorla (:ekr) from comment #56)
> > That's not the rationale.
> 
> I will just cite:
> 
> (In reply to Eric Rescorla (:ekr) from comment #16)
> > Why don't you instead rename it first and then land this, thus removing the
> > need to change this code twice.
> 
> (In reply to Eric Rescorla (:ekr) from comment #56)
> > And given that this bug is almost a year old and the
> > rename still hasn't happened, I think it's quite appropriate for me to be
> > concerned that these patches not land on some "eventually we'll rename"
> > basis.
> 
> There have been other patches in-progress to rename in other places, and it
> didn't seem worth disrupting those in the meantime.

I'm not sure why you think this some kind of gotcha. The issue isn't primarily the
number of times the code is changed but rather avoiding having the code be in
a confusing state, and the simplest way to do so is to only have one CL that
puts it in the desired state. However, what I don't want to happen is that *this* patch
lands and then there are all sorts of excuses why this naming issue then can't
be fixed. Given that it's been almost a year, I think that's a valid concern.

So, I ask again: what is the state of the other renaming patches and when will they
land?

Comment 60

[Nathan Froyd [:froydnj]]

Comment on attachment 8777193 [details] [diff] [review]
bug-1197328.patch - with #include IntegerPrintfMacros.h

Review of attachment 8777193 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, with the note below, and the caveat that this patch needs to be redone with bug 1293384.  I've needinfo'd you in the other bug about doing the renaming.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +650,2 @@
>        temp,
> +      "%" PRIu64 " (id= %" PRIu64 " url=%s)",

Nit: this has a space between "id=" and the value, which the original did not have.

Comment 61

[Igor]

Attachment: bug-1197328.patch - using SprintfLiteral()

Hi,

I am attaching a new patch here, including all the mentioned issues.
Also, since we renamed the function to SprintfLiteral(), this patch already uses the new naming.

If there is anything else that can be done to improve this task, please let me know. If there is any other bug in which I can work, feel free to suggest one. Someone already pointed that maybe other modules would need to implement the use of this function.

Comment 62

[Nathan Froyd [:froydnj]]

Comment on attachment 8781815 [details] [diff] [review]
bug-1197328.patch - using SprintfLiteral()

Review of attachment 8781815 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating the patch!

Comment 63

[Igor]

Thanks! If everybody is ok with the patch, can someone push it to Try server for the tests please?

Comment 64

[Nils Ohlmeier [:drno]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba97894f5f36

Comment 65

[Igor]

It seems that the testes went ok. Can I mark this patch for checkin?

Comment 66

[Nathan Froyd [:froydnj]]

(In reply to Igor from comment #65)
> It seems that the testes went ok. Can I mark this patch for checkin?

Done, thank you!

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e99ab4ba80
Remove PR_snprintf calls in media/{webrtc,mtransport}/. r=froydnj

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7e99ab4ba80