search

ericmckean / snappy

Automatically exported from code.google.com/p/snappy
Other
0 stars 0 forks source link

'Compress' compress input length should call 'GetAppendBuffer' at first. #51

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
I think code like these matches the semantics of 'Sink::GetAppendBuffer' and 
'Sink::Append'

size_t Compress(Source* reader, Sink* writer) {
    size_t written = 0;
    int N = reader->Available();
    char ulength[Varint::kMax32];
    char* dest= writer->GetAppendBuffer(Varint::kMax32,ulength);
    char* p = Varint::Encode32(dest,N);
    writer->Append(dest,p-dest);
    written += (p - dest);

Original issue reported on code.google.com by dirtysal...@gmail.com on 10 Oct 2011 at 8:09

GoogleCodeExporter commented 9 years ago 
I'm not sure if I understand this bug. Are you saying GetAppendBuffer() _must_ 
be called before Append()? There is no such demand in the API.

Original comment by se...@google.com on 14 Oct 2011 at 10:11

GoogleCodeExporter commented 9 years ago 
:(.sorry,just I don't know how to change the issue 'Type', the default value of 
'Type' is 'Defect'. Actually It's not a bug. again sorry.:(.

the semantics of 'GetAppendBuffer(length,scratch)' is telling app to give out a 
buffer for the next 'Append',the parameter 'scratch' is just a optional buffer, 
app can return 'scratch' directly or give out the internal buffer for the next 
'Append'. 

so here is the problem.when start Compress, you plan to write 'ulength' , if 
you call 'Append' directly you didn't give app a chance to give out the 
internal buffer . so I think here you should call 'GetAppendBuffer' at first 
then call 'Append', like the rest code of 'Compress'

and if you write Compress like this
========================
size_t Compress(Source* reader, Sink* writer) {
    size_t written = 0;
    int N = reader->Available();
    char ulength[Varint::kMax32];
    char* dest= writer->GetAppendBuffer(Varint::kMax32,ulength);
    char* p = Varint::Encode32(dest,N);
    writer->Append(dest,p-dest);
    written += (p - dest);

=======================
then 'UncheckedByteArraySink' could be implemented like this. It's much elegant 
and consistent

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
void UncheckedByteArraySink::Append(const char* data, size_t n) {
  dest_ += n;
}

char* UncheckedByteArraySink::GetAppendBuffer(size_t len, char* scratch) { 
  return dest_;                                                                                          
}
<<<<<<<<<<<<<<<<<<<<<<<<

Original comment by dirtysal...@gmail.com on 14 Oct 2011 at 10:30

GoogleCodeExporter commented 9 years ago 
Ah, I'm sorry, but we can't do that; this is effectively changing the API to 
make sure you _must_ return an append buffer, and that is not appropriate for 
all kinds of sinks (there are sinks that are not included in the open-source 
distribution).

Closing.

Original comment by se...@google.com on 18 Oct 2011 at 1:29