Summary: | Use NakedPtr<Exception>& to return exception results. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | andersca, benjamin, darin, fpizlo, ggaren, keith_miller, kling, mmirman, msaboff | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Mark Lam
2015-06-10 20:55:01 PDT
Created attachment 254706 [details]
candidate patch.
Comment on attachment 254706 [details] candidate patch. View in context: https://bugs.webkit.org/attachment.cgi?id=254706&action=review > Source/JavaScriptCore/runtime/ExceptionResult.h:35 > +template <typename T> > +class Ptr { > +public: I think this template will be useful in WTF as well. e.g. NodeRareData could use this for m_childNodeList. > Source/JavaScriptCore/runtime/ExceptionResult.h:61 > +class ExceptionResult : public Ptr<Exception> { I think it's better to call this ExceptionPtr or ExceptionResultPtr so that overloading operators are easier to reason about in callsites. It seems like we can just use typedef?? Or maybe we should just use Ptr<Exceptoin> directly since Ptr seems useful elsewhere. Since there is a use for the self initializing pointer class in the rest of the code base, I think it is a good idea to implement the pointer class in WTF. However, it turns out that we can't use Ptr as the class name because there is a collision with a typedef in MacTypes.h. In consultation with Anders and Andreas, we settled on NakedPtr as a good name to go with for now. naked_ptr is one of the alternate names for this concept in the proposal to add this pointer class to the C++ stdlib (see http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3514.pdf). Will upload an updated patch for review again (since I've made substantial changes to the patch). Created attachment 254851 [details]
patch 2: using NakedPtr<Exception>.
Comment on attachment 254851 [details] patch 2: using NakedPtr<Exception>. View in context: https://bugs.webkit.org/attachment.cgi?id=254851&action=review > Tools/TestWebKitAPI/TestWebKitAPI.vcxproj/TestWebKitAPI.vcxproj:369 > -</Project> > \ No newline at end of file > +</Project> Will remove this unnecessary change before landing. > Tools/TestWebKitAPI/TestWebKitAPI.vcxproj/TestWebKitAPI.vcxproj.filters:181 > -</Project> > \ No newline at end of file > +</Project> Will remove this unnecessary change before landing. Comment on attachment 254851 [details] patch 2: using NakedPtr<Exception>. View in context: https://bugs.webkit.org/attachment.cgi?id=254851&action=review > Source/WTF/wtf/NakedPtr.h:57 > + // This conversion operator allows implicit conversion to bool but not to other integer types. > + typedef T* (NakedPtr::*UnspecifiedBoolType); > + operator UnspecifiedBoolType() const { return m_ptr ? &NakedPtr::m_ptr : nullptr; } Instead of this, please add an explicit operator bool() const. Created attachment 254962 [details]
patch 3: applied Anders' feedback.
Thanks for the feedback. The patch has been updated.
Comment on attachment 254962 [details] patch 3: applied Anders' feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=254962&action=review R=me but please consider the two suggestions. > Source/WTF/wtf/NakedPtr.h:33 > +template <typename T> class NakedPtr { Please add a comment up here about what this is for. In particular, that it initializes the pointer to null. > Source/WTF/wtf/NakedPtr.h:53 > + bool operator!() const { return !m_ptr; } Can we use UnspecifiedNullType here, to prevent accidental coercions to null? (In reply to comment #6) > Comment on attachment 254851 [details] > patch 2: using NakedPtr<Exception>. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254851&action=review > > > Source/WTF/wtf/NakedPtr.h:57 > > + // This conversion operator allows implicit conversion to bool but not to other integer types. > > + typedef T* (NakedPtr::*UnspecifiedBoolType); > > + operator UnspecifiedBoolType() const { return m_ptr ? &NakedPtr::m_ptr : nullptr; } > > Instead of this, please add an explicit operator bool() const. Oh, I saw that I had the opposite feedback. Why is explicit operator bool() better? (In reply to comment #8) > > Source/WTF/wtf/NakedPtr.h:53 > > + bool operator!() const { return !m_ptr; } > > Can we use UnspecifiedNullType here, to prevent accidental coercions to null? operator! will only be called when you use the ! operator. (In reply to comment #9) > > Oh, I saw that I had the opposite feedback. Why is explicit operator bool() > better? I find that it's easier to read while still having the same guarantees agents accidental conversion. (In reply to comment #10) > (In reply to comment #8) > > > > Source/WTF/wtf/NakedPtr.h:53 > > > + bool operator!() const { return !m_ptr; } > > > > Can we use UnspecifiedNullType here, to prevent accidental coercions to null? > > operator! will only be called when you use the ! operator. Sorry, I meant to put that comment on "operator bool()". By mistake, I put it on operator!(). > > (In reply to comment #9) > > > > Oh, I saw that I had the opposite feedback. Why is explicit operator bool() > > better? > > I find that it's easier to read while still having the same guarantees > agents accidental conversion. Does this mean that we should eliminate all of our uses of UnspecifiedNullType? (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > Oh, I saw that I had the opposite feedback. Why is explicit operator bool() > > > better? > > > > I find that it's easier to read while still having the same guarantees > > agents accidental conversion. > > Does this mean that we should eliminate all of our uses of > UnspecifiedNullType? I am pretty sure it does. UnspecifiedNullType was a workaround for the lack of the "explicit operator bool()" feature. Created attachment 254966 [details]
patch for landing.
For the record, in the previous patch, I neglected to add the "explicit" keyword to operator bool(). That has been remedied in this patch.
Thanks for the reviews. Landed in r185608: <http://trac.webkit.org/r185608>. |