Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(25)

Issue 10790149: Add awesome_ptr<>. (Closed)

Created:
8 years, 5 months ago by awong
Modified:
8 years ago
CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org, Ami GONE FROM CHROMIUM, kinuko+watch, hans, tzik+watch_chromium.org
Visibility:
Public.

Description

Add awesome_ptr<>. awesome_ptr<> provides instrumentation in the codebase that allows for small-footprint, real-usage testing scenarios. Long term, it can help improve the debugging chops of maintainers on the chromium project. Data is collected via the crash reporting mechanism. awesome_ptr<> is safe for multi-threaded usage. BUG=none TEST=int main(void) {char* c = (char*)0x1;awesome_ptr<char> p(c); if(p) printf("%p\n", (char*)p); return 0; } compiled and printed 0x1.

Patch Set 1 #

Total comments: 20

Patch Set 2 : address comments, make const correct and 64-bit safe. #

Patch Set 3 : Fixed type conversion. #

Total comments: 10

Patch Set 4 : Address shess's comments. Add unittests and fix readability issue with single gotos. #

Total comments: 8

Patch Set 5 : Fix test names. #

Patch Set 6 : Add missing .h, and fix bug that levin@ spotted. #

Patch Set 7 : stack allocated for speed (per levin@) #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+114 lines, -0 lines) Patch
M base/base.gyp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M base/base.gypi View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
A base/memory/awesome_ptr.h View 1 2 3 4 5 6 1 chunk +75 lines, -0 lines 5 comments Download
A base/memory/awesome_ptr_unittest.cc View 1 2 3 4 5 6 1 chunk +37 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
awong
8 years, 5 months ago (2012-07-24 18:23:33 UTC) #1
dmichael (off chromium)
LATM https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h#newcode8 base/memory/awesome_ptr.h:8: // small-footpring, real-usage testing scenarios. Long term, it ...
8 years, 5 months ago (2012-07-24 18:30:45 UTC) #2
Avi (use Gerrit)
https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h#newcode8 base/memory/awesome_ptr.h:8: // small-footpring, real-usage testing scenarios. Long term, it can ...
8 years, 5 months ago (2012-07-24 18:32:04 UTC) #3
dglazkov
I would not be exaggerating one bit if I state that this patch is full ...
8 years, 5 months ago (2012-07-24 18:37:23 UTC) #4
dmichael (off chromium)
https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h#newcode18 base/memory/awesome_ptr.h:18: awesome_ptr(T* ptr) : ptr_(ptr) {} On 2012/07/24 18:32:04, Avi ...
8 years, 5 months ago (2012-07-24 19:04:35 UTC) #5
awong
+willchan and darin for OWNERS https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1/base/memory/awesome_ptr.h#newcode8 base/memory/awesome_ptr.h:8: // small-footpring, real-usage testing ...
8 years, 5 months ago (2012-07-24 19:09:16 UTC) #6
Avi (use Gerrit)
LATM; my concerns were all addressed.
8 years, 5 months ago (2012-07-24 19:14:15 UTC) #7
Scott Hess - ex-Googler
Needs unit tests. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome_ptr.h#newcode4 base/memory/awesome_ptr.h:4: Header guards. https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome_ptr.h#newcode13 base/memory/awesome_ptr.h:13: // awesome_ptr<> ...
8 years, 5 months ago (2012-07-24 19:22:14 UTC) #8
awong
PTAL https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/6002/base/memory/awesome_ptr.h#newcode4 base/memory/awesome_ptr.h:4: On 2012/07/24 19:22:15, shess wrote: > Header guards. ...
8 years, 5 months ago (2012-07-24 20:47:11 UTC) #9
Avi (use Gerrit)
https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome_ptr_unittest.cc File base/memory/awesome_ptr_unittest.cc (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome_ptr_unittest.cc#newcode15 base/memory/awesome_ptr_unittest.cc:15: TEST(AwesomePtrTest, PointerFunctionality) { This tests the bool conversion, so ...
8 years, 5 months ago (2012-07-24 20:52:13 UTC) #10
awong
https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome_ptr_unittest.cc File base/memory/awesome_ptr_unittest.cc (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/memory/awesome_ptr_unittest.cc#newcode15 base/memory/awesome_ptr_unittest.cc:15: TEST(AwesomePtrTest, PointerFunctionality) { On 2012/07/24 20:52:14, Avi wrote: > ...
8 years, 5 months ago (2012-07-24 20:56:53 UTC) #11
Scott Hess - ex-Googler
LATM. guess you just need an OWNERS review. https://chromiumcodereview.appspot.com/10790149/diff/3002/base/base.gyp File base/base.gyp (right): https://chromiumcodereview.appspot.com/10790149/diff/3002/base/base.gyp#newcode412 base/base.gyp:412: 'memory/awesome_ptr_unittest.cc', ...
8 years, 5 months ago (2012-07-24 21:06:40 UTC) #12
awong
okay, last revision. Per offline discussion with levin@, added in stack allocated version of ptr ...
8 years, 5 months ago (2012-07-24 22:45:16 UTC) #13
levin
https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome_ptr.h#newcode32 base/memory/awesome_ptr.h:32: new (ptr) T*((T*)ptr_); // stack allocated for speed. Thanks. ...
8 years, 5 months ago (2012-07-24 22:45:26 UTC) #14
darin (slow to review)
I don't think this new class has sufficient test coverage.
8 years, 5 months ago (2012-07-24 23:51:10 UTC) #15
Ryan Sleevi
https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome_ptr.h#newcode46 base/memory/awesome_ptr.h:46: goto awww; drive-by nit: I'm a little concerned about ...
8 years, 5 months ago (2012-07-25 02:37:06 UTC) #16
hans
https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome_ptr.h File base/memory/awesome_ptr.h (right): https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome_ptr.h#newcode32 base/memory/awesome_ptr.h:32: new (ptr) T*((T*)ptr_); // stack allocated for speed. shouldn't ...
8 years, 5 months ago (2012-07-25 09:10:28 UTC) #17
jln (very slow on Chromium)
Another neat trick: you should make use of non canonical AMD64 addresses, that way you ...
8 years, 5 months ago (2012-07-26 01:43:19 UTC) #18
srvasude1
8 years, 4 months ago (2012-08-09 20:20:44 UTC) #19
https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome...
File base/memory/awesome_ptr.h (right):

https://chromiumcodereview.appspot.com/10790149/diff/1007/base/memory/awesome...
base/memory/awesome_ptr.h:60: ptr_ = (T*)((unsigned long)ptr_ &
~(MAKE_MASK(unsigned long)));
Small nit: There is no break statement here. The switch statement should either
have no break statements or each case should have a break statement for
consistency.

Powered by Google App Engine
This is Rietveld 408576698