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

Issue 279883002: Prevent calls to ReturnValue::Set with pointer-valued types. (Closed)

Created:
6 years, 7 months ago by vogelheim
Modified:
6 years, 7 months ago
Reviewers:
dcarney
CC:
v8-dev, Paweł Hajdan Jr.
Visibility:
Public.

Description

Prevent calls to ReturnValue::Set with pointer-valued types. [2nd try, after the previous version broke the build] Currently, this code will compile: SomePointer* p = ...; ReturnValue r = ...; r.Set(p); What happens is that ReturnValue::Set has no pointer-ish overloads, but a bool one, and hence C++ will convert the pointer to a bool and use the Set(bool) overload. In other words, the example above is equivalent to: r.Set(p ? true : false); Which probably isn't what the author had in mind. This change adds a Set(void*) overload whose body forces a compile error, to prevent this from happening inadvertently. The only use of this indeed turned out to be an error. (Said error was fixed/removed in crrev.com/267393002.) Why was crrev.com/240013004 reverted? The orginal version compiled fine on gcc (+ MSVC), but not on clang. There's no clang try-bots, but the ASAN-based buildbots used clang and hence the build broke. I'm slightly unsure on why, but clang - unlike those other compilers - eagerly compiled the non-compilable setter, which predictably broke. Now, the non-compilable setter uses the same template logic that all other, comparable cases use. I've tried 'make qc' with both gcc and clang versions. BUG= R=dcarney@chromium.org Committed: https://code.google.com/p/v8/source/detail?r=21228

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -0 lines) Patch
M include/v8.h View 2 chunks +11 lines, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
vogelheim
6 years, 7 months ago (2014-05-09 14:38:20 UTC) #1
dcarney
lgtm
6 years, 7 months ago (2014-05-09 15:13:33 UTC) #2
vogelheim
6 years, 7 months ago (2014-05-09 15:36:57 UTC) #3
Message was sent while issue was closed.
Committed patchset #1 manually as r21228 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698