Closed Bug 1418743 Opened 5 years ago Closed 5 years ago

Stop using Encoding::ForName in FallbackEncoding.cpp

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(1 file)

No description provided.
Priority: -- → P2
Comment on attachment 8929812 [details]
Bug 1418743 - Stop using Encoding::ForName in FallbackEncoding.cpp.

https://reviewboard.mozilla.org/r/201036/#review210750

::: dom/encoding/FallbackEncoding.cpp:35
(Diff revision 2)
> +                   const nsACString& aKey)
> +{
> +  const nsCString& flat = PromiseFlatCString(aKey);
> +  size_t index;
> +  if (!BinarySearchIf(aProperties, 0, ArrayLength(aProperties),
> +                      [flat](const EncodingProp& aProperty)

Does `[flat]` end up running a copy constructor on every comparison? Would `[&flat]` be better?

r+ with `[&flat]` if that works or with `[flat]` and an explanation why `[&flat]` isn't appropriate.
Attachment #8929812 - Flags: review?(hsivonen) → review+
Comment on attachment 8929812 [details]
Bug 1418743 - Stop using Encoding::ForName in FallbackEncoding.cpp.

https://reviewboard.mozilla.org/r/201036/#review210750

> Does `[flat]` end up running a copy constructor on every comparison? Would `[&flat]` be better?
> 
> r+ with `[&flat]` if that works or with `[flat]` and an explanation why `[&flat]` isn't appropriate.

This is roughly equivalent to:
```
struct Comparator {
  const nsCString flat;
  int operator ()(const EncodingProp& aProperty) const
  { return flat.Compare(aProperty.mKey); }
};
if (!BinarySearchIf(aProperties, 0, ArrayLength(aProperties),
    Comparator{flat}))
```
The copy constructor will be invoked only once when the closure is created, not everytime it is called.

But you are right about that capturing `flat` by reference is safe here and more efficient. I'll change this to [&flat].
(In reply to Masatoshi Kimura [:emk] from comment #4)
> The copy constructor will be invoked only once when the closure is created,
> not everytime it is called.

I see.

> But you are right about that capturing `flat` by reference is safe here and
> more efficient. I'll change this to [&flat].

Thanks.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/59adfaf5e09f
Stop using Encoding::ForName in FallbackEncoding.cpp. r=hsivonen
https://hg.mozilla.org/mozilla-central/rev/59adfaf5e09f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Blocks: 1425217
Assignee: nobody → VYV03354
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.