[llvm] r185851 - [ADT/NullablePtr] Allow implicit conversion of NullablePtr<OtherT> -> NullablePtr<T> if OtherT is derived from T.

[David Blaikie]

On Tue, Jul 9, 2013 at 11:48 AM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> On Jul 8, 2013, at 7:37 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Mon, Jul 8, 2013 at 7:31 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>> On Jul 8, 2013, at 5:31 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>> On Mon, Jul 8, 2013 at 5:08 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>>>> On Jul 8, 2013, at 4:05 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>
>>>>> On Mon, Jul 8, 2013 at 2:51 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>
>>>>> wrote:
>>>>>
>>>>> On Jul 8, 2013, at 12:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>
>>>>> On Mon, Jul 8, 2013 at 12:12 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>
>>>>> wrote:
>>>>>
>>>>> Author: akirtzidis
>>>>> Date: Mon Jul  8 14:12:01 2013
>>>>> New Revision: 185851
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=185851&view=rev
>>>>> Log:
>>>>> [ADT/NullablePtr] Allow implicit conversion of NullablePtr<OtherT> ->
>>>>> NullablePtr<T> if OtherT is derived from T.
>>>>>
>>>>>
>>>>> Might be nicer to do this for any case where SrcT* is implicitly
>>>>> convertible DestT* (such as adding cv qualifiers or converting to
>>>>> void*).
>>>>>
>>>>>
>>>>> cv qualifiers look like are working, void* is not.
>>>>> In general, we need to add the equivalent of C++11's is_convertible to
>>>>> "llvm/Support/type_traits.h" and use it here.
>>>>>
>>>>>
>>>>> Shall we do that then?
>>>>>
>>>>> (your current implementation also disables this ctor for NullablePtr's
>>>>> of non-class type (NullablePtr<int> for example), I think... that
>>>>> seems unreasonable)
>>>>>
>>>>>
>>>>> Feel free to enhance it :-)
>>>>
>>>> Alternatively, this code doesn't seem to have any uses inside the llvm
>>>> codebase (at least not clang, llvm, compiler-rt, libcxx - maybe in
>>>> lldb?) nor any tests. Should we just remove it instead? (I assume you
>>>> have a use-case, or you wouldn't've been motivated to update it - is
>>>> that use case somewhere in the llvm project (broadly, across all
>>>> subprojects) or out of tree?)
>>>
>>> It is used out of tree.
>>
>> It might be more suitable for it to live along with the code that's
>> consuming it, I'd imagine. (that'd be my preference, at least - though
>> if it's really necessary for it to be here, it should probably at
>> least have unit tests)
>
> Unit tests for this are overkill IMO,

Even if it's just compile tests to check that some of the examples I
gave work correctly/as expected, that doesn't seem unreasonable to me
(if only to enshrine/document the intended semantics).

> we could just start using it in new llvm/clang APIs.

That it's been in the codebase for 2 years & still (?) has zero uses
seems to indicate that that might not be desired & should probably
just be removed.

>
>>
>> - David
>>
>>>
>>>>
>>>> - David
>>>>
>>>>>
>>>>> -Argyrios
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/include/llvm/ADT/NullablePtr.h
>>>>>
>>>>> Modified: llvm/trunk/include/llvm/ADT/NullablePtr.h
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/NullablePtr.h?rev=185851&r1=185850&r2=185851&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/include/llvm/ADT/NullablePtr.h (original)
>>>>> +++ llvm/trunk/include/llvm/ADT/NullablePtr.h Mon Jul  8 14:12:01 2013
>>>>> @@ -14,6 +14,7 @@
>>>>> #ifndef LLVM_ADT_NULLABLEPTR_H
>>>>> #define LLVM_ADT_NULLABLEPTR_H
>>>>>
>>>>> +#include "llvm/Support/type_traits.h"
>>>>> #include <cassert>
>>>>> #include <cstddef>
>>>>>
>>>>> @@ -25,8 +26,17 @@ namespace llvm {
>>>>> template<class T>
>>>>> class NullablePtr {
>>>>> T *Ptr;
>>>>> +  struct PlaceHolder {};
>>>>> +
>>>>> public:
>>>>> NullablePtr(T *P = 0) : Ptr(P) {}
>>>>> +
>>>>> +  template<typename OtherT>
>>>>> +  NullablePtr(NullablePtr<OtherT> Other,
>>>>> +              typename enable_if<
>>>>> +                is_base_of<T, OtherT>,
>>>>> +                PlaceHolder
>>>>> +              >::type = PlaceHolder()) : Ptr(Other.getPtrOrNull()) {}
>>>>>
>>>>> bool isNull() const { return Ptr == 0; }
>>>>> bool isNonNull() const { return Ptr != 0; }
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>
>