Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667993003:
Don't use LazyInstance in chrome/browser/image_decoder.* (Closed)
DescriptionDon't use LazyInstance in chrome/browser/image_decoder.*
R=thakis@chromium.org
BUG=686866
Review-Url: https://codereview.chromium.org/2667993003
Cr-Commit-Position: refs/heads/master@{#448384}
Committed: https://chromium.googlesource.com/chromium/src/+/d69ee9d7f4329d536d02cda7b00046b102c55fc1
Patch Set 1 #
Total comments: 3
Patch Set 2 : auto* #
Depends on Patchset: Messages
Total messages: 14 (4 generated)
lgtm Out of interest, did you compare assembly for the LazyInstance and the static version on some platforms? https://codereview.chromium.org/2667993003/diff/1/chrome/browser/image_decode... File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/2667993003/diff/1/chrome/browser/image_decode... chrome/browser/image_decoder.cc:96: static auto image_decoder = new ImageDecoder(); style guide says "auto*" for pointers
Oh I think you maybe never sent this out? Ignore if you didn't want a stamp -- I just went through my pending reviews on rietveld since my inbox is unusable atm
On 2017/02/02 22:13:19, Nico wrote: > lgtm > > Out of interest, did you compare assembly for the LazyInstance and the static > version on some platforms? I checked Windows MSVC here: https://bugs.chromium.org/p/chromium/issues/detail?id=686866#c21 The summary seems to be "it depends". > > https://codereview.chromium.org/2667993003/diff/1/chrome/browser/image_decode... > File chrome/browser/image_decoder.cc (right): > > https://codereview.chromium.org/2667993003/diff/1/chrome/browser/image_decode... > chrome/browser/image_decoder.cc:96: static auto image_decoder = new > ImageDecoder(); > style guide says "auto*" for pointers
https://codereview.chromium.org/2667993003/diff/1/chrome/browser/image_decode... File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/2667993003/diff/1/chrome/browser/image_decode... chrome/browser/image_decoder.cc:96: static auto image_decoder = new ImageDecoder(); On 2017/02/02 22:13:19, Nico wrote: > style guide says "auto*" for pointers Huh, I didn't know that. I have some places to fix.
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2667993003/#ps20001 (title: "auto*")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2667993003/diff/1/chrome/browser/image_decode... File chrome/browser/image_decoder.cc (right): https://codereview.chromium.org/2667993003/diff/1/chrome/browser/image_decode... chrome/browser/image_decoder.cc:96: static auto image_decoder = new ImageDecoder(); On 2017/02/04 01:44:32, scottmg wrote: > On 2017/02/02 22:13:19, Nico wrote: > > style guide says "auto*" for pointers > > Huh, I didn't know that. I have some places to fix. [I was going to make a CL with a link to the style guide to make other places match this, but I can't find anything in a guide that says that. In the guide https://google.github.io/styleguide/cppguide.html#auto itself auto is suggested for `new`, but it doesn't mention preferring auto*. Way far down in the "Another discussion thread" link on chromium-cpp, Daniel suggested it and a couple people agreed, but I would say it's not terribly obvious or codified. I personally think it's reasonable but I'm not sure I feel strongly enough to wage war upon cxx@ again.]
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486411751705340,
"parent_rev": "7a7fbf7c5016a85e4610b9e2fe653a77807e932b", "commit_rev":
"d69ee9d7f4329d536d02cda7b00046b102c55fc1"}
Message was sent while issue was closed.
Description was changed from ========== Don't use LazyInstance in chrome/browser/image_decoder.* R=thakis@chromium.org BUG=686866 ========== to ========== Don't use LazyInstance in chrome/browser/image_decoder.* R=thakis@chromium.org BUG=686866 Review-Url: https://codereview.chromium.org/2667993003 Cr-Commit-Position: refs/heads/master@{#448384} Committed: https://chromium.googlesource.com/chromium/src/+/d69ee9d7f4329d536d02cda7b000... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d69ee9d7f4329d536d02cda7b000...
Message was sent while issue was closed.
Huh, http://chromium-cpp.appspot.com/ says it in the for loop section but not in general. I don't remember if we wanted this only in loops, or in general. I guess I'll stop asking for this then, thanks! On Mon, Feb 6, 2017 at 3:46 PM, <scottmg@chromium.org> wrote: > > https://codereview.chromium.org/2667993003/diff/1/chrome/ > browser/image_decoder.cc > File chrome/browser/image_decoder.cc (right): > > https://codereview.chromium.org/2667993003/diff/1/chrome/ > browser/image_decoder.cc#newcode96 > chrome/browser/image_decoder.cc:96: static auto image_decoder = new > ImageDecoder(); > On 2017/02/04 01:44:32, scottmg wrote: > > On 2017/02/02 22:13:19, Nico wrote: > > > style guide says "auto*" for pointers > > > > Huh, I didn't know that. I have some places to fix. > > [I was going to make a CL with a link to the style guide to make other > places match this, but I can't find anything in a guide that says that. > In the guide https://google.github.io/styleguide/cppguide.html#auto > itself auto is suggested for `new`, but it doesn't mention preferring > auto*. > > Way far down in the "Another discussion thread" link on chromium-cpp, > Daniel suggested it and a couple people agreed, but I would say it's not > terribly obvious or codified. I personally think it's reasonable but I'm > not sure I feel strongly enough to wage war upon cxx@ again.] > > https://codereview.chromium.org/2667993003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2017/02/06 21:01:36, Nico wrote: > Huh, http://chromium-cpp.appspot.com/ says it in the for loop section but > not in general. I don't remember if we wanted this only in loops, or in > general. I guess I'll stop asking for this then, thanks! Ah, I didn't notice that one, thanks. Seems sensible there. > > On Mon, Feb 6, 2017 at 3:46 PM, <mailto:scottmg@chromium.org> wrote: > > > > > https://codereview.chromium.org/2667993003/diff/1/chrome/ > > browser/image_decoder.cc > > File chrome/browser/image_decoder.cc (right): > > > > https://codereview.chromium.org/2667993003/diff/1/chrome/ > > browser/image_decoder.cc#newcode96 > > chrome/browser/image_decoder.cc:96: static auto image_decoder = new > > ImageDecoder(); > > On 2017/02/04 01:44:32, scottmg wrote: > > > On 2017/02/02 22:13:19, Nico wrote: > > > > style guide says "auto*" for pointers > > > > > > Huh, I didn't know that. I have some places to fix. > > > > [I was going to make a CL with a link to the style guide to make other > > places match this, but I can't find anything in a guide that says that. > > In the guide https://google.github.io/styleguide/cppguide.html#auto > > itself auto is suggested for `new`, but it doesn't mention preferring > > auto*. > > > > Way far down in the "Another discussion thread" link on chromium-cpp, > > Daniel suggested it and a couple people agreed, but I would say it's not > > terribly obvious or codified. I personally think it's reasonable but I'm > > not sure I feel strongly enough to wage war upon cxx@ again.] > > > > https://codereview.chromium.org/2667993003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Looks like I wasn't completely imagining the auto* bit: https://bugs.chromium.org/p/chromium/issues/detail?id=554600#c73 On Mon, Feb 6, 2017 at 4:16 PM, <scottmg@chromium.org> wrote: > On 2017/02/06 21:01:36, Nico wrote: > > Huh, http://chromium-cpp.appspot.com/ says it in the for loop section > but > > not in general. I don't remember if we wanted this only in loops, or in > > general. I guess I'll stop asking for this then, thanks! > > Ah, I didn't notice that one, thanks. Seems sensible there. > > > > > On Mon, Feb 6, 2017 at 3:46 PM, <mailto:scottmg@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/2667993003/diff/1/chrome/ > > > browser/image_decoder.cc > > > File chrome/browser/image_decoder.cc (right): > > > > > > https://codereview.chromium.org/2667993003/diff/1/chrome/ > > > browser/image_decoder.cc#newcode96 > > > chrome/browser/image_decoder.cc:96: static auto image_decoder = new > > > ImageDecoder(); > > > On 2017/02/04 01:44:32, scottmg wrote: > > > > On 2017/02/02 22:13:19, Nico wrote: > > > > > style guide says "auto*" for pointers > > > > > > > > Huh, I didn't know that. I have some places to fix. > > > > > > [I was going to make a CL with a link to the style guide to make other > > > places match this, but I can't find anything in a guide that says that. > > > In the guide https://google.github.io/styleguide/cppguide.html#auto > > > itself auto is suggested for `new`, but it doesn't mention preferring > > > auto*. > > > > > > Way far down in the "Another discussion thread" link on chromium-cpp, > > > Daniel suggested it and a couple people agreed, but I would say it's > not > > > terribly obvious or codified. I personally think it's reasonable but > I'm > > > not sure I feel strongly enough to wage war upon cxx@ again.] > > > > > > https://codereview.chromium.org/2667993003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2667993003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
