Add GetBytes method to write to pre-allocated buffer #37
Conversation
ghost
| } | ||
|
|
||
| public static int GetBytes(Variant value, BinaryMode mode, byte[] buffer, int bufferIndex) | ||
| { |
There was a problem hiding this comment.
I think that bufferOffset would be a better name for the last argument.
Also, var below?
| writer.Write(value); | ||
| } | ||
|
|
||
| return (int)ms.Position; |
There was a problem hiding this comment.
Is there any reason to manipulate the type here, rather than using the more natural long throughout these methods?
(I realise we are unlikely to be writing very many 2GB variants...)
There was a problem hiding this comment.
The MemoryBuffer class is inconsistent. The offset/index you can pass in is an int, not sure why Position would be a long then. I mainly cast it here so the client doesn't have to do it. Also arrays with indexes > max int are quite hard to deal with. You need to use Array.GetValue() instead of the bracket indexers.
| m_stream.Write(BitConverter.GetBytes((int)m_mode), 0, 4); | ||
| } | ||
|
|
||
| // This doesn't seem to support Unicode properly |
There was a problem hiding this comment.
Weren't you going to change this so that it will throw for an out-of-range character, rather than silently mangling it?
There was a problem hiding this comment.
I'm a bit reluctant to change behaviour, it might break existing code
| m_filter.Write(arg, 0, arg.Length); | ||
| } | ||
|
|
||
| private void Write(bool arg) |
There was a problem hiding this comment.
While you're here, could you add some more brackets in the call to Write() below, just to be sure? :)
| Write(bytes, true); | ||
| } | ||
|
|
||
| private static int GetByteCountString(string arg) |
There was a problem hiding this comment.
If you're going to suffix all of the GetByteCount methods, should you update the Write methods to have the same? Either way works, but I think we should probably be consistent about it.
There was a problem hiding this comment.
Ok, will do - I don't like overloads.
| DataColumn col = arg.Columns[i]; | ||
| colTypes[i] = VariantPrimitiveBase.TypeToEnum(col.DataType); | ||
| colNames[i] = col.ColumnName; | ||
| } |
There was a problem hiding this comment.
Allocating temporary arrays seems rather unnecessary
There was a problem hiding this comment.
Yea, I guess I converted this code too mechanically...
| var writeCount = sizeof(Int32); | ||
|
|
||
| switch (type) | ||
| { |
There was a problem hiding this comment.
Couldn't this borrow GetWriteCounterDelegate()?
There was a problem hiding this comment.
Possibly, but I think it would be a bit weird because the delegate only supports types that can be used in an array. I was trying to keep the counting code as similar to the write code as possible.
1 participant
No description provided.