incubator-stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Travis Vitek" <Travis.Vi...@roguewave.com>
Subject RE: svn commit: r647908 - in /stdcxx/trunk/tests: self/0.printf.cpp src/fmt_defs.h src/printf.cpp
Date Mon, 14 Apr 2008 21:23:58 GMT
 

Martin Sebor wrote 
>
>Travis Vitek wrote:
>>  
>> 
>>> Martin Sebor wrote:
>>>
>>> vitek@apache.org wrote:
>>>> URL: http://svn.apache.org/viewvc?rev=647908&view=rev
>>>> Log:
>>>> 2008-04-14  Travis Vitek  <vitek@roguewave.com>
>>>>
>>>> 	STDCXX-857
>>>> 	* tests/src/fmt_defs.h: Add flag to struct Buffer to indicate
>>>> 	who owns the allocated buffer.
>>>
>>> I wonder if this flag is really necessary give the maxsize
>>> member of Buffer whose purpose was to avoid reallocation
>>> (the implementation may have fallen short of that goal):
>>> http://svn.apache.org/viewvc?view=rev&revision=381880
>>>
>> 
>> I don't think I can use the maxsize field for this, but I 
>> have a bit of confusion about this maxsize member.
>> 
>> If maxsize != _RWSTD_UINT_MAX, then the user has provided 
>> the necessary format specifier to tell us not to grow the
>> buffer past a set length. We are still allowed to grow the
>> buffer, just not past this limit. Since we are allowed to
>> grow the buffer, even if maxsize is set, I need some way to
>> indicate that I don't know who allocated the buffer or
>> where it came from.
>
>Wouldn't (*pbufsize == maxsize) in that case so that _rw_bufcat()
>would never need to allocate new space to begin with (or it would
>fail trying).
>

That only tells me if I've reached the maximum allowed buffer size. It
tells me nothing about where the *pbufsize bytes were allocated from,
and that is exactly what I need, right?

I mean there are cases where (*pbufsize != maxsize) and we don't want to
deallocate the buffer. Consider this case which is taken from the test
library...

  static int
  _rw_vsystem (const char *cmd, va_list va)
  {
      RW_ASSERT (0 != cmd);

      char buffer [256];
      char *buf = buffer;

      size_t bufsize = sizeof buffer;

      rw_vasnprintf (&buf, &bufsize, cmd, va);

      // <snip>
   }

We definitely don't want `buf' to be deallocated, that much is clear.
But how can `maxsize' possibly tell us anything about `buf'?

>
>> 
>> Now I know that I could have stolen a bit from maxsize for a 
>> flag, but I didn't want to do that. The additional size_t
>> field didn't seem all that expensive considering that these
>> buffer objects are allocated on the stack in a call to a
>> formatted output function.
>
>Right. I'm not worried about the space overhead of a new data
>member but rather about duplicating the same information in
>two places, and the two becoming out of sync.
>

Agreed.

>> 
>> I just noticed that the code in fmt_bits.cpp uses rw_asnprintf()
>> expecting that the buffer reallocation done internally would free
>> the input buffer if necessary. Previously that might have caused 
>> the assert that my change was supposed to fix. Now that code
>> might leak the buffer. So I'm putting together a patch to fix
>> that now.
>
>Yeah, the code there (the "%{+}" directives) is kind of clunky.
>
>> 
>> In fixing that, I noticed that we have a function rw_vasnprintf() is
>> defined in printf.cpp, but it isn't declared in rw_printf.h. That
>> function is declared in each of tests/src/{ctype, driver, exception,
>> process}.cpp. For a clean fix I'd like to use the function in
>> test/src/fmt_bits.cpp, but I'd prefer to not forward declare 
>> in another place. Is there some motivation to not declare the
>> function in the appropriate place. The only reason I can think
>> of to not do so would be introducing <stdarg.h> into one of the
>> test headers, but that doesn't seem like it should be a problem
>> because we are doing it in all of the previously mentioned source
>> files.
>
>That's the reason. <rw_printf.h> isn't supposed to depend on any
>libc (or C++ standard library) header so that it can be included
>in tests without introducing namespace pollution. This should be
>documented somewhere. But instead of declaring the function in
>every driver source file that uses it maybe we could declare it
>in a driver-only header?
>

Got it. I'm just going to fwd declare it this time. I'll file an
enhancement for this to be done at a later time.

>Martin
>

Mime
View raw message