[llvm-commits] [llvm] r135365 - in /llvm/trunk: docs/ProgrammersManual.html include/llvm/ADT/TinyPtrVector.h

[Chris Lattner]

On Jul 18, 2011, at 1:25 AM, Frits van Bommel wrote:
>> +//===----------------------------------------------------------------------===//
>> +//
>> +// This file defines the Type class.
> 
> s/Type/TinyPtrVector/

Zapped, thanks.  Too many thoughts about types are something :)

>> +/// TinyPtrVector - This class is specialized for cases where there are
>> +/// normally 0 or 1 element in a vector, but is general enough to go beyond that
>> +/// when required.
>> +///
>> +/// NOTE: This container doesn't allow you to store a null pointer into it.
> 
> This restriction could be lifted by using a null VecTy* as the empty
> value instead of any null pointer (what empty() currently checks for)
> or a null EltTy (what can actually happen).
> 
> But if none of the use cases you have in mind for this require it this
> might not be worth the added complexity.

Interesting! This approach didn't occur to me.  I think it would be useful to support null pointers: if nothing else, it takes away a gotcha that will bite people at runtime if they're unaware of it.

One issue: if you swap the elements of the pointer union, you'll break begin/end, because they depend on being able to take the address of the single-pointer case and not have the low bit set.

>> +  /// empty() - This vector can be empty if it contains no element, or if it
>> +  /// contains a pointer to an empty vector.
> 
> This doesn't seem doc-comment worthy. It talks about implementation
> details the user shouldn't care about and which aren't mentioned in
> any of the other documentation.
> All other comments of this kind are inside the methods, which is where
> this one belongs too.

Fixed.

>> +  unsigned size() const {
>> +    if (empty())
>> +      return 0;
>> +    if (Val. template is<EltTy>())
> 
> There's an extra space before 'template', which doesn't match the
> style used in most other cases in this file.

Fixed.  Gotta love C++ :)

Thanks for the review Frits!

-Chris