Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
510323002:
Disable scoped_refptr operator T* on Linux. (Closed)
DescriptionDisable scoped_refptr operator T* on Linux builds.
The implicit conversion operator is being removed, because of the
surprises it can cause. For example, one might write:
scoped_refptr<Foo> CreateFoo();
to ensure that callers hold the result in a scoped_refptr. But the
implicit conversion to T* means that a caller can accidentally hold
the result in a Foo* and the compiler won't catch it.
Since there are a large number of dependencies on this implicit
conversion, the cleanup is being conducted by a combination of
automated tooling and manual cleanup. It is also being staged on a
platform by platform basis. All known dependencies on the implicit
conversion on Linux (but not ChromeOS) have been removed, so this
patch disables the conversion operator on Linux builds to prevent
new dependencies on the implicit conversion.
Finally, to help facilitate this transition, temporary operator
overloads for comparison have been added. These will eventually be
removed.
BUG=110610
R=jyasskin@chromium.org, rsleevi@chromium.org, willchan@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/664a17fefa8bc4b50763765281039afaf0db7b0d
Patch Set 1 #Patch Set 2 : . #
Total comments: 17
Patch Set 3 : Remove bool coercion #Patch Set 4 : Address comments #
Total comments: 6
Patch Set 5 : Address comments #Patch Set 6 : Exclude OS_CHROMEOS #Messages
Total messages: 22 (0 generated)
dcheng@chromium.org changed reviewers: + jamesr@chromium.org, jyasskin@chromium.org, rsleevi@chromium.org, willchan@chromium.org
Sending this out for an initial request for comments. I think it'd make things simpler for the transition to support comparisons between scoped_refptr<T> and T* (I think a fair amount of code expects this, in DCHECK/CHECK/etc, since this works for scoped_ptr too), but there's still a lot of manual fixups that need to land (I have 15+ reviews in flight still). I'm happy to re-examine the necessity of this once the codebase is a bit cleaner.
seems reasonable, albiet shared_ptr in C++11 land only allows comparisons between shared<U> and shared<T>, and not T*... Further comments below. https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:393: } For comparison, shared_ptr (C++11) defines == != < > <= >= (For comparing between shared<T> and shared<U>) 1) Why no < for *? 2) Why no >, <=, >= for scoped<U> (line 345) https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:407: const scoped_refptr<T>& p) { shared_ptr defines in 20.7.2.2.11, so seems reasonable.
https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:393: } On 2014/08/28 17:32:20, Ryan Sleevi wrote: > For comparison, shared_ptr (C++11) defines > == > != > < > > > <= > >= > > (For comparing between shared<T> and shared<U>) > > 1) Why no < for *? > 2) Why no >, <=, >= for scoped<U> (line 345) It wasn't needed for things to compile. I think only things like std::map/std::set end up using this, which only needs <. I can certainly add it if it's desired (I know shared_ptr defines all these), but I opted to only add what was necessary. https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:407: const scoped_refptr<T>& p) { On 2014/08/28 17:32:20, Ryan Sleevi wrote: > shared_ptr defines in 20.7.2.2.11, so seems reasonable. In that case, I won't bother matching scoped_ptr's behavior then? Printing 0 vs 1 is pretty useless =)
https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:344: #if defined(DISABLE_SCOPED_REFPTR_CONVERSION_OPERATOR) c++11 have these operators defined for shared_ptr<> so we should probably just define them without guards. it seems weird but that's life https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:355: // STL is fun. comment isn't useful https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:376: bool operator==(const scoped_refptr<T>& lhs, const U* rhs) { i'd really prefer not to add this and instead insert .get() where needed. do you have an exact count of how many places would need these? how many can be handled by a machine translation? this is a very tricky API to add and it'll be hard to remove once we add it https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:404: // "Just Work" for some very twisted definiton of "Just Working". this comment isn't terribly useful https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:406: BASE_EXPORT std::ostream& operator<<(std::ostream& out, EXPORT on an inline function makes no sense. there's no symbol in base.dll to export https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:407: const scoped_refptr<T>& p) { On 2014/08/28 17:37:15, dcheng (OOO) wrote: > On 2014/08/28 17:32:20, Ryan Sleevi wrote: > > shared_ptr defines in 20.7.2.2.11, so seems reasonable. > > In that case, I won't bother matching scoped_ptr's behavior then? Printing 0 vs > 1 is pretty useless =) Can't you just rely on the fallback byte-by-byte printer in gtest? That works for every type and doesn't require us to compile this template-bloaty stream code into every translation unit that #includes ref_counted (which will be almost all of them) https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted_... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted_... base/memory/ref_counted_unittest.cc:65: #if 0 just delete the code, it'll live on in git until it's time to revive it. #if 0'd code is confusing
https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:344: #if defined(DISABLE_SCOPED_REFPTR_CONVERSION_OPERATOR) On 2014/08/28 18:19:42, jamesr wrote: > c++11 have these operators defined for shared_ptr<> so we should probably just > define them without guards. it seems weird but that's life I need to wrap them in this guard, because otherwise, resolving the right overload for == would be ambiguous. https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:355: // STL is fun. On 2014/08/28 18:19:43, jamesr wrote: > comment isn't useful Done. https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:376: bool operator==(const scoped_refptr<T>& lhs, const U* rhs) { On 2014/08/28 18:19:42, jamesr wrote: > i'd really prefer not to add this and instead insert .get() where needed. do you > have an exact count of how many places would need these? how many can be handled > by a machine translation? this is a very tricky API to add and it'll be hard to > remove once we add it I'll try to get a more accurate count once there aren't as many other compile failures. It's difficult to get an exact measure when I still need to apply several hundred lines of manual fixups. https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:404: // "Just Work" for some very twisted definiton of "Just Working". On 2014/08/28 18:19:42, jamesr wrote: > this comment isn't terribly useful I've removed the comment (it was to try to document why I used bool coercion below) https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:406: BASE_EXPORT std::ostream& operator<<(std::ostream& out, On 2014/08/28 18:19:42, jamesr wrote: > EXPORT on an inline function makes no sense. there's no symbol in base.dll to > export Done. https://codereview.chromium.org/510323002/diff/20001/base/memory/ref_counted.... base/memory/ref_counted.h:407: const scoped_refptr<T>& p) { On 2014/08/28 18:19:43, jamesr wrote: > On 2014/08/28 17:37:15, dcheng (OOO) wrote: > > On 2014/08/28 17:32:20, Ryan Sleevi wrote: > > > shared_ptr defines in 20.7.2.2.11, so seems reasonable. > > > > In that case, I won't bother matching scoped_ptr's behavior then? Printing 0 > vs > > 1 is pretty useless =) > > Can't you just rely on the fallback byte-by-byte printer in gtest? That works > for every type and doesn't require us to compile this template-bloaty stream > code into every translation unit that #includes ref_counted (which will be > almost all of them) This is for the logging macros (CHECK/DCHECK) and friends.
With respect to "bloaty IO stream includes" - as evidenced, we've already got them in ref_counted.h :) https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted.... base/memory/ref_counted.h:395: std::ostream& operator<<(std::ostream& out, const scoped_refptr<T>& p) { Don't you need iosfwd included when NDEBUG isn't set (because you don't have base/logging.h included)
I'm planning on letting y'all argue this out and mostly rubberstamping the decision, unless somehow I feel violently opposed. But there are enough people here that I respect greatly, so I am opting out of spending my time here. Thanks for handling this guys! dcheng: In case I don't notice it myself, please ping me out of band for a rubberstamp when the dust settles. On Thu, Aug 28, 2014 at 11:42 AM, <rsleevi@chromium.org> wrote: > With respect to "bloaty IO stream includes" - as evidenced, we've already > got > them in ref_counted.h :) > > > https://codereview.chromium.org/510323002/diff/60001/base/ > memory/ref_counted.h > File base/memory/ref_counted.h (right): > > https://codereview.chromium.org/510323002/diff/60001/base/ > memory/ref_counted.h#newcode395 > base/memory/ref_counted.h:395: std::ostream& operator<<(std::ostream& > out, const scoped_refptr<T>& p) { > Don't you need iosfwd included when NDEBUG isn't set (because you don't > have base/logging.h included) > > https://codereview.chromium.org/510323002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #5 (id:80001) has been deleted
OK, so I ran a pass this morning to see how many instances failed without the comparison operators between T* and scoped_refptr<T>. I found 58 instances which would need to be fixed, across ~10 files (I eyeballed this latter number). I thought this would be a lot higher. Given that, fixing this isn't too bad... However, the number of diffs left for making Chrome compile with the current patch is very small (see https://codereview.chromium.org/508213003). The current diffs in the megapatch are all things I think can be TBRed, whereas changing the comparisons (most of them due to using std::find()) is something I'd want to get an actual review for. Given that, would you be OK letting this patch proceed as is, with the understanding that the comparisons will be cleaned up afterwards? It makes things a lot easier for me, since I'm constantly rebasing my mega patch and trying to peel off manageable chunks to land. Proceeding incrementally would simplify things for me.
I'm ok with that plan. https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted_... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted_... base/memory/ref_counted_unittest.cc:64: // Temporarily disabled while the conversion operator is being removed. better to delete and then re-add, imo
I'm OK with this approach. I'm gonna punt to jyasskin, as I'm not as smart as he or will, but this works for me. I'd much rather get this disabled and us moving forward than worry about painting the operator bikeshed red because the STL does. https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted.... base/memory/ref_counted.h:18: #if defined(OS_LINUX) 1) Shouldn't you explicitly include build/build_config.h ? 2) This includes ChromeOS, doesn't it?
I'm honestly not sure about the OS_CHROMEOS vs OS_LINUX thing. I know I would be surprised if more than one was defined. I'll try to find some more info. https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted.h File base/memory/ref_counted.h (right): https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted.... base/memory/ref_counted.h:18: #if defined(OS_LINUX) On 2014/09/04 21:29:04, Ryan Sleevi wrote: > 1) Shouldn't you explicitly include build/build_config.h ? > 2) This includes ChromeOS, doesn't it? 1) Done. 2) I thought they were mutually exclusive. A random comment in https://code.google.com/p/chromium/codesearch#chromium/src/base/linux_util.cc... leaves me to believe this is the case. I'll run this patch through the CrOS trybots to see what happens. https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted.... base/memory/ref_counted.h:395: std::ostream& operator<<(std::ostream& out, const scoped_refptr<T>& p) { On 2014/08/28 18:42:51, Ryan Sleevi wrote: > Don't you need iosfwd included when NDEBUG isn't set (because you don't have > base/logging.h included) Done. https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted_... File base/memory/ref_counted_unittest.cc (right): https://codereview.chromium.org/510323002/diff/60001/base/memory/ref_counted_... base/memory/ref_counted_unittest.cc:64: // Temporarily disabled while the conversion operator is being removed. On 2014/09/04 at 21:28:01, jamesr wrote: > better to delete and then re-add, imo Done.
On 2014/09/04 21:36:18, dcheng (OOO) wrote:
> I'm honestly not sure about the OS_CHROMEOS vs OS_LINUX thing. I know I would
be
> surprised if more than one was defined. I'll try to find some more info.
>
OS_CHROMEOS and OS_LINUX are both 1 in a chromeos build, surprising as that is.
To verify, run gyp with -Dchromeos=1 and observe out/Debug/obj/base/base.ninja:
defines = -DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DDISABLE_NACL $
-DCHROMIUM_BUILD -DCR_CLANG_REVISION=214024 -DTOOLKIT_VIEWS=1 $
-DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1 -DUSE_PANGO=1 $
-DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_X11=1 $
-DUSE_XI2_MT=2 -DIMAGE_LOADER_EXTENSION=1 -DENABLE_REMOTING=1 $
-DENABLE_WEBRTC=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY $
-DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DUSE_UDEV -DENABLE_EGLIMAGE=1 $
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1 $
-DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 $
-DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 $
-DCLD2_DATA_SOURCE=static -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1 $
-DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 $
-DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 $
-DENABLE_SERVICE_DISCOVERY=1 -DUSE_SYMBOLIZE -DUSE_NSS=1 $
-DOS_CHROMEOS=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS $
-DBASE_IMPLEMENTATION -DDYNAMIC_ANNOTATIONS_ENABLED=1 $
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -D_GLIBCXX_DEBUG=1
see how that sets -DOS_CHROMEOS=1. However, OS_LINUX is decided by
build/build_config.h based on only __linux__, which is defined.
To prove it, add this:
#if OS_CHROMEOS==1 && OS_LINUX==1
#error hello
#endif
and build with chromeos. You get
../../base/scoped_native_library.cc:8:2: error: hello
#error hello
^
1 error generated.
whee!
lgtm, but I don't mean that to override any of the other reviewers.
OK, I've fixed this to exclude ChromeOS. Thanks for the pointers.
In case it's not clear, I don't own base, but lgtm
Can you update the CL description to explain why this is getting disabled only on Linux, and also why ChromeOS is excluded? LGTM
On 2014/09/04 at 23:17:36, willchan wrote: > Can you update the CL description to explain why this is getting disabled only on Linux, and also why ChromeOS is excluded? > > LGTM I've updated the patch description to describe the motivation for the removal and process.
Perfect, thanks.
Message was sent while issue was closed.
Committed patchset #6 (id:120001) manually as 664a17f (tree was closed).
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/664a17fefa8bc4b50763765281039afaf0db7b0d Cr-Commit-Position: refs/heads/master@{#293459} |
