On 11/30/05, Peter N. Lundblad <peter@famlundblad.se> wrote:
> On Tue, 29 Nov 2005 dionisos@tigris.org wrote:
[ log snipped ]
> You didn't mention the change to svn_subst_copy_and_translate3 here.
Indeed. I didn't want to commit that because of Julian's comments...
So, I'll remove the change (and update the log message).
>
> > + * Read operations from and write operations to the stream
> > + * perform the same operation: if @a expand is @c FALSE, both
> > + * contract keywords.
> > + *
> Say that the stream is allocated in @a pool. Is it OK to mix read and
> write operations?
Ok. Yes, read and write operations on the same stream are supported in
any order.
> > + b->newline_buf[0] = '\0';
> > + b->newline_buf[1] = '\0';
>
> Does something depend on the characters to be zero?
I don't think there are dependencies, no.
> > + b->newline_off = 0;
> > + for (i = 0; i < SVN_KEYWORD_MAX_LEN; i++)
> > + b->keyword_buf[i] = '\0';
> Why not use memset instead of this explicit loop? And same question as
> above.
I don't think there are dependencies, no.
> > + b->keyword_off = 0;
> > + b->src_format[0] = '\0';
> > + b->src_format[1] = '\0';
>
> And here.
I don't think there are dependencies, no. (Yes, I'll remove the initialization.)
> > +/* Translate eols and keywords of a 'chunk' of characters BUF of size BUFLEN
> > + * according to the settings and state stored in baton B.
> > + *
> > + * Write output to stream D.
> > + *
>
> One-letter variable names are sometimes convenient, but make the code
> harder to read. We use b in many places for baton, but could you please
> change d to dst or something?
>
> > + * To finish a series of chunk translations, flush all buffers by calling
> > + * this routine with a NULL value for BUF.
>
> "Use POOL for temporary allocations".
>
> > - /* We're in the boring state; look for interest characters.
> > - * For lack of a memcspn(), manually skip past NULs. */
> > + /* We're in the boring state; look for interest characters. */
> > len = 0;
> > - while (1)
> > - {
> > - len += strcspn (p + len, interesting);
> > - if (p[len] != '\0' || p + len == buf + readlen)
> > - break;
> > - len++;
> > - }
> > +
> > + /* Fake strcspn() with a length parameter, strncspn if you like,
> > + because we're not sure the buffer ends with a NUL character.
> > +
>
> I understand that you did this change because before we had a sentinel
> value at the end of the buffer... I think the new comment is misleading,
> because what we actually wanted was some memcspn, because a) we don't have
> a terminating NUL and b) we want to allow NUL characters in the data.
>
> > + Also, reject NUL characters explicitly, since index()
> > + considers them part of the string argument, but we don't...
>
> What do you mean by "reject"? I'd say "skip" for the reason stated above.
>
> > + */
> > + while ((p + len) < end
> > + && (! p[len] || ! index (interesting, p[len])))
>
> Looking in my manpages on Linux, it says that index is a BSD extension,
> while strchr is part of ISO C. Why not use that instead?
Much better idea.
> > +struct translated_stream_baton
>
> Docstring.
>
> > +{
> > + svn_stream_t *stream;
> > + struct translation_baton *in_baton, *out_baton;
> > + svn_boolean_t written;
> > + svn_stringbuf_t *readbuf;
> > + char *buf;
> > + apr_size_t readbuf_off;
> > + apr_pool_t *pool;
> > +} translated_stream_baton;
> > +
> > +
> > +static svn_error_t *
> > +translated_stream_read (void *baton,
> > + char *buffer,
> > + apr_size_t *len)
> > +{
> > + struct translated_stream_baton *b = baton;
> > + apr_size_t readlen = SVN_STREAM_CHUNK_SIZE;
>
> I think we should get rid of SVN_STREAM_CHUNK_SIZE because it is a very
> big buffer, but that shouldn't be part of this commit, I guess.
You mean in this case, or everywhere in the code? Just getting rid of
the large constant won't help: sometimes you want to allocate in
somewhat larger buffers.
> > + apr_size_t unsatisfied = *len;
> > + apr_size_t off = 0;
> > + apr_pool_t *iterpool;
> > + svn_pool_clear (b->pool);
> > +
> > + iterpool = svn_pool_create (b->pool);
> > + while (readlen == SVN_STREAM_CHUNK_SIZE && unsatisfied > 0)
> > + {
> > + apr_size_t to_copy;
> > +
> > + /* fill read buffer, if necessary */
> > + if (! (b->readbuf_off < b->readbuf->len))
> > + {
> > + svn_stream_t *buf_stream;
> > +
> > + svn_stringbuf_setempty (b->readbuf);
> > + SVN_ERR (svn_stream_read (b->stream, b->buf, &readlen));
> > + buf_stream = svn_stream_from_stringbuf (b->readbuf, iterpool);
> > +
> > + SVN_ERR (translate_chunk (buf_stream, b->in_baton, b->buf,
> > + readlen, iterpool));
> > +
> > + if (readlen != SVN_STREAM_CHUNK_SIZE)
> > + SVN_ERR (translate_chunk (buf_stream, b->in_baton, NULL, 0,
> > + iterpool));
> > +
> > + SVN_ERR (svn_stream_close (buf_stream));
> > + }
> > +
> > + /* Satisfy from the read buffer */
> > + to_copy = (b->readbuf->len > unsatisfied)
> > + ? unsatisfied : b->readbuf->len;
> > + memcpy (buffer + off, b->readbuf->data + b->readbuf_off, to_copy);
> > + off += to_copy;
> > + b->readbuf_off += to_copy;
> > + unsatisfied -= to_copy;
> > + }
>
> iterpool is never cleared.
>
> > +
> > + *len -= unsatisfied;
> >
> ...or destroyed for that matter:-)
Grr! that makes me feel so sloppy! Ok. working on it.
> > return SVN_NO_ERROR;
> > }
> >
> ...
> > +
> > +svn_stream_t *
> > +svn_subst_stream_translated (svn_stream_t *stream,
> > + const char *eol_str,
> > + svn_boolean_t repair,
> > + apr_hash_t *keywords,
> > + svn_boolean_t expand,
> > + apr_pool_t *pool)
> ...
>
> > + /* Setup the baton fields */
> > + baton->stream = stream;
> > + baton->in_baton
> > + = create_translation_baton (eol_str, repair, keywords, expand, pool);
> > + baton->out_baton
> > + = create_translation_baton (eol_str, repair, keywords, expand, pool);
> > + baton->written = FALSE;
> > + baton->readbuf = svn_stringbuf_create ("", pool);
>
> > + baton->readbuf_off = 0;
> > + baton->buf = apr_palloc (pool, SVN_STREAM_CHUNK_SIZE + 1);
> > + baton->pool = svn_pool_create (pool);
> > +
> The above may leave 256K of buffers allocated in pool after using this
> stream. Normally it is the callers responsibility to handle pool memory,
> but this might be too much. My preferred solution is to use a smaller
> buffer size (say 8K or something), for several reasons. If you don't want
> to do that now, maybe you should use a subpool here? Maybe use
> baton->pool; it doesn't seem to be used except for allocating the
> iterpool.
>
>
> > +
> > +svn_error_t *
> > +svn_subst_translate_stream3 (svn_stream_t *s, /* src stream */
> > + svn_stream_t *d, /* dst stream */
> > + const char *eol_str,
> > + svn_boolean_t repair,
> > + apr_hash_t *keywords,
> > + svn_boolean_t expand,
> > + apr_pool_t *pool)
> > +{
> > + apr_pool_t *subpool = svn_pool_create (pool);
> > + apr_pool_t *iterpool = svn_pool_create (subpool);
> > + struct translation_baton *baton;
> > + apr_size_t readlen = SVN_STREAM_CHUNK_SIZE;
> > + char *buf = apr_palloc (subpool, SVN_STREAM_CHUNK_SIZE);
>
> Here, someone thought using the main poool for the big buffer wasn't
> appropriate:-)
>
> Other than that, nice work, Erik!
>
Heh! Thanks!
bye,
Erik.
Received on Wed Nov 30 20:35:12 2005