llvm dev - May 2009 - [LLVMdev] Removing std::vector from APIs (was Re: Mutating the elements of a ConstantArray)

2009/4/1 Chris Lattner <clattner at apple.com>:
> As far API design goes, we're in a mixed state.  I'd strongly
prefer
> to get rid of std::vector from the various interfaces, f.e. creating a
> constant array currently requires passing in an std::vector.  For
> these sorts of interfaces, we should migrate to passing in a "Constant
> *const* / unsigned" pair.  This allows use with a C array, a
> SmallVector, std::vector, or any other container with sequential
> storage.
I've made a start on this for the constructors of StructType and
FunctionType. I've attached the patch so far, which is just enough to
build lib/VMCore/. Before I continue with it:

1. Is this still the way you want to go?
2. Do you care about breaking out-of-tree code that creates struct or
function types? I'm happy to convert cfe and llvm-gcc myself.
3. Are there any logistical problems with committing a wide-reaching
patch like this?
3. Any comments on the patch itself?

Thanks,
Jay.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.novector
Type: application/octet-stream
Size: 11187 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090514/1716a3d2/attachment.obj>

On May 14, 2009, at 4:56 AM, Jay Foad wrote:
> 2009/4/1 Chris Lattner <clattner at apple.com>:
>> As far API design goes, we're in a mixed state.  I'd strongly
prefer
>> to get rid of std::vector from the various interfaces, f.e.  
>> creating a
>> constant array currently requires passing in an std::vector.  For
>> these sorts of interfaces, we should migrate to passing in a  
>> "Constant
>> *const* / unsigned" pair.  This allows use with a C array, a
>> SmallVector, std::vector, or any other container with sequential
>> storage.
>
> I've made a start on this for the constructors of StructType and
> FunctionType. I've attached the patch so far, which is just enough to
> build lib/VMCore/. Before I continue with it:
Great!
> 1. Is this still the way you want to go?
Yes
> 2. Do you care about breaking out-of-tree code that creates struct or
> function types? I'm happy to convert cfe and llvm-gcc myself.
Nope, we expect people with out of tree code to update themselves.
> 3. Are there any logistical problems with committing a wide-reaching
> patch like this?
I don't think so.
> 3. Any comments on the patch itself?
The one major thing to be aware of is that it isn't safe to use &V[0]  
when V is an empty std::vector (though it is safe for smallvector).   
When converting code, just be careful so that this doesn't break {} or  
void().

One minor thing is that StructType::get(NULL, 0) looks somewhat  
strange to me.  How about adding a zero-argument version of "get" that
returns the empty struct?  That would allow code to use  
StructType::get().

Thanks for working on this Jay!

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090514/e0b51d25/attachment.html>

> 3. Any comments on the patch itself?
>
> The one major thing to be aware of is that it isn't safe to use
&V[0] when V
> is an empty std::vector
Oh dear. That's a bit of a flaw in the plan. I suppose the solution is
to switch to SmallVector whenever this might be a problem.

I'm a bit concerned that any new &empty[0] problems that are
introduced will go unnoticed. With GNU libstdc++ they aren't diagnosed
unless you build with -D_GLIBCXX_DEBUG (or ENABLE_EXPENSIVE_CHECKS=1).

For now I'm testing with ENABLE_EXPENSIVE_CHECKS=1, and it is indeed
catching lots of errors!
> (though it is safe for smallvector).
DR 464 proposes a new data() method. I'd suggest implementing that in
SmallVector, instead of relying on the relaxed checking in
operator[]().

http://www.open-std.org/jtc1/sc22/wg21/docs/lwg-defects.html#464

GNU libstdc++'s vector already has this data() method, but I suppose
we can't rely on in it being in std::vector in general.
> One minor thing is that StructType::get(NULL, 0) looks somewhat strange to
> me.  How about adding a zero-argument version of "get" that
returns the
> empty struct?  That would allow code to use StructType::get().
OK, I've done this for both StructType and FunctionType.

I'll post some patches on llvm-commits.

Thanks,
Jay.