reduce use of nsTextFormatter.cpp

NEW
Assigned to

Status

()

P3
normal
2 years ago
10 months ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
From https://bugzilla.mozilla.org/show_bug.cgi?id=1060419#c166

xpcom/glue/nsTextFormatter.cpp has another fork of the NSPR printf code.
It would be nice to remove this in favor of reusing the code that will
land in mozglue in bug 1060419.  It would simultaneously be good to get
compiler printf format checking here.

One difficulty is that nsTextFormatter only has a char16_t interface.

Also, at least GCC (I don't know about clang) won't handle a printf attribute
here; see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64862
(Assignee)

Comment 1

2 years ago
I looked a little into completely removing nsTextFormatter.
There are two main problems.

One, there is code that gets a format string (as a char16_t*) from elsewhere, like:
https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/parser/htmlparser/nsExpatDriver.cpp#918-923

Two, nsTextFormatter has special handling for floating point formatting:
https://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTextFormatter.cpp#372
... and maybe SVG is relying on either this or some other oddity:
https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/dom/svg/SVGNumberList.cpp#33,34
(Assignee)

Comment 2

a year ago
Maybe not every use has to be converted.  See bug 1388789 for a reason to keep nsTextFormatter around.

One other issue that has to be looked out for is %s handling.
In nsTextFormatter, this is UTF-8.  However, at least AppendPrintf is written to use ASCII here.

In theory %S is also a problem but in practice it seems that the only uses of this are
in code that wouldn't be converted anyhow.
(Assignee)

Comment 3

a year ago
Changing SVG to use Printf without pulling over the weird, special, nsTextFormatter %g code
passes all the SVG tests locally.  But it's hard to be confident in this kind of change.
Priority: -- → P3
(Assignee)

Comment 4

a year ago
I don't think this class can be removed now; but we can reduce the use of it by 
preferring the compiler-checked variants where possible.
Assignee: nobody → ttromey
Summary: remove duplicated code from nsTextFormatter.cpp → reduce use of nsTextFormatter.cpp
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
mozreview-review
Comment on attachment 8927344 [details]
Bug 1331760 - remove unused includes of nsTextFormatter.h;

https://reviewboard.mozilla.org/r/198648/#review212958
Attachment #8927344 - Flags: review?(nfroyd) → review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8927345 [details]
Bug 1331760 - convert dom/base from nsTextFormatter to AppendPrintf;

https://reviewboard.mozilla.org/r/198650/#review212960

::: dom/base/nsJSEnvironment.cpp:1728
(Diff revision 1)
>      if (sPostGCEventsToConsole) {
>        nsCOMPtr<nsIConsoleService> cs =
>          do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>        if (cs) {
>          cs->LogStringMessage(msg.get());
>        }
>      }
>      if (gCCStats.mFile) {
>        fprintf(gCCStats.mFile, "%s\n", NS_ConvertUTF16toUTF8(msg).get());
>      }

I wonder if it'd be better to format `msg` as `nsCString` to attempt to avoid conversions here in the common case?  I don't know what the common case is, but I *assume* it'd be logging it to a file, rather than printing it to the console?
Attachment #8927345 - Flags: review?(nfroyd) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8927346 [details]
Bug 1331760 - convert dom/svg from nsTextFormatter to AppendPrintf;

https://reviewboard.mozilla.org/r/198652/#review212964

Assuming you agree with my understanding of the floating-point issue mentioned below (and so for all the other floating-point printing areas in the rest of the patch), I think this is good to go.

::: dom/svg/SVGLength.cpp:26
(Diff revision 1)
>  
>  void
>  SVGLength::GetValueAsString(nsAString &aValue) const
>  {
> -  nsTextFormatter::ssprintf(aValue, u"%g", (double)mValue);
> +  aValue.Truncate();
> +  aValue.AppendPrintf("%g", (double)mValue);

So, assuming I understand this correctly:

Before, `nsTextFormatter::ssprintf` for `%g` would ultimately dispatch through `::sprintf`, i.e. the system `printf` implementation.

After, `AppendPrintf` for `%g` will dispatch to `SprintfLiteral`, which dispatches to `::sprintf` or similar, i.e. the system `printf` implementation.

So there should be no difference between these two ways of printing, correct?  I think at some point in the past, we had system-independent printing for floats in `nsTextFormatter` through `PR_snprintf`...but we made the decision to change that and go through the system `printf`...is that correct?  So we already made the hard decision about having the system do the formatting here, right?  (Is it embarassing for the maintainer of this code to be asking all these questions?)
Attachment #8927346 - Flags: review?(nfroyd) → review+

Comment 12

a year ago
mozreview-review
Comment on attachment 8927347 [details]
Bug 1331760 - convert xpcom/components from nsTextFormatter to AppendPrintf;

https://reviewboard.mozilla.org/r/198654/#review212966
Attachment #8927347 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 13

a year ago
(In reply to Nathan Froyd [:froydnj] (Limited time-only Austin reviews; void where prohibited) from comment #11)

> Before, `nsTextFormatter::ssprintf` for `%g` would ultimately dispatch
> through `::sprintf`, i.e. the system `printf` implementation.
> 
> After, `AppendPrintf` for `%g` will dispatch to `SprintfLiteral`, which
> dispatches to `::sprintf` or similar, i.e. the system `printf`
> implementation.
> 
> So there should be no difference between these two ways of printing,
> correct?  I think at some point in the past, we had system-independent
> printing for floats in `nsTextFormatter` through `PR_snprintf`...but we made
> the decision to change that and go through the system `printf`...is that
> correct?  So we already made the hard decision about having the system do
> the formatting here, right?  (Is it embarassing for the maintainer of this
> code to be asking all these questions?)

No, not embarrassing at all.

Unfortunately the strange special case is still in nsTextFormatter.
Speaking of embarrassment - I can't tell if I should be embarrassed that this makes
no sense to me, or if we should all be collectively embarrassed that this exists:
https://dxr.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0/xpcom/string/nsTextFormatter.cpp#364-406
I guess this is converting to exponential notation in some cases?
Maybe all this code should go away in favor of sprintf.
(Assignee)

Comment 14

11 months ago
Never did set the NI flag on this, but I think we still need to discuss it a bit, so setting it now.
Flags: needinfo?(nfroyd)
OK, I think I misunderstood the code, specifically:

https://dxr.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0/xpcom/string/nsTextFormatter.cpp#377

this bit is printing integers (exponents, I think?  The Linux printf man page helpfully says, "g ... [converts] in style f or e", so I think this is handling the case where exponents are needed.  The actual floating-point formatting is done here:

https://dxr.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0/xpcom/string/nsTextFormatter.cpp#288

I think that means that if we switch from nsTextFormatter::ssprintf to AppendPrintf for floating-point format directives, we are now using the system floating-point formatter, rather than some platform-independent version from NSPR.  And I don't think we want to do that.  Does that sound right?
Flags: needinfo?(nfroyd)
You need to log in before you can comment on or make changes to this bug.