Closed Bug 1341880 Opened 3 years ago Closed 3 years ago

PrintfTarget should not emit a trailing \0

Categories

(Core :: mozglue, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

Attachments

(1 file)

Currently PrintfTarget emits a trailing \0.
See https://dxr.mozilla.org/mozilla-central/rev/b06968288cff469814bf830aa90f1c84da490f61/mozglue/misc/Printf.cpp#884

However this is only really of use to SprintfState; other users don't want this.
It seems better to emit to not emit this, and to have SprintfState deal with
its own needs.
Assignee: nobody → ttromey
Blocks: 1334307
Comment on attachment 8842199 [details]
Bug 1341880 - do not emit trailing \0 in PrintfState;

https://reviewboard.mozilla.org/r/115862/#review118044

::: mozglue/misc/Printf.h:123
(Diff revision 1)
>      ~SprintfState() {
>          this->free_(mBase);
>      }
>  
> +    bool vprint(const char* format, va_list ap_list) {
> +        return mozilla::PrintfTarget::vprint(format, ap_list) && append("", 1);

I can see bystanders raising eyebrows on the append("", 1) (which is perfectly correct, just non obvious to casual readers ; especially, I can see someone asking themselves where that comes from, look at blame, see the corresponding changeset, and be confused that the patch replaces an emit("\0", 1) with an append("", 1)). Can you add a comment as to why it works?
Attachment #8842199 - Flags: review?(mh+mozilla) → review+
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/133ad85684a1
do not emit trailing \0 in PrintfState; r=glandium
https://hg.mozilla.org/mozilla-central/rev/133ad85684a1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.