stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Martin Sebor <se...@roguewave.com>
Subject Re: svn commit: r421918 - in /incubator/stdcxx/trunk/tests: include/rw_alloc.h src/alloc.cpp
Date Mon, 17 Jul 2006 23:52:48 GMT
Farid Zaripov wrote:
>  > -----Original Message-----
>  > From: Farid Zaripov [mailto:FaridZ@kyiv.vdiweb.com]
>  > Sent: Monday, July 17, 2006 6:54 PM
>  > To: stdcxx-dev@incubator.apache.org
>  > Subject: RE: svn commit: r421918 - in
>  > /incubator/stdcxx/trunk/tests: include/rw_alloc.h src/alloc.cpp
> 
>  > > The error handling in the file isn't very robust: we shouldn't just
>  > > assert that the system calls succeeded; we should print out
>  > an error
>  > > message and either exit with a non-zero exit status or
>  > return a value
>  > > indicating a failure (if possible). If the former, the mechanism to
>  > > use is
>  > > rw_error() and/or rw_fatal(). The %m and %{#m} directives let you
>  > > format the text of the error message corresponding to the value of
>  > > errno and the name of the EXXX variable, respectively.
>  >
>  >   Done. The diff file is attached.
> 
>   I have accidentally sent the letter from Outlook and attachment have 
> been cut out.

Okay, this looks (almost) good :) I should have been more clear
about what I meant above. Sorry. Let me try to clarify:

Issuing an error message (and either exiting with a non zero exit
status, or returning a failure code to the caller) is appropriate
in the case of failed system or library (such as libc or libpthread)
function calls due to runtime error conditions that might be rare
but that cannot be prevented by the design of a program (e.g.,
having run out of resources such as memory, file descriptors, file
mappings, etc.).

Assertions are the appropriate mechanism to verify preconditions
and postconditions mandated by the design of a piece of code. If
the code assumes that a page size is a multiple of two and there's
no way the code could reasonably deal with the violation of the
assumption, an assertion (RW_ASSERT) is better than a more graceful
handling of the condition (such as the above) since it indicates
a logic flaw in the design of the program. Ditto when an internal
data structure becomes corrupted as a result of a bug in our code.

So given this distinction, ...

> Index: alloc.cpp
[...]
> @@ -243,20 +245,25 @@
>          static const size_t pagemask = GETPAGESIZE () - 1;
>  
>          // check that pagesize is power of 2
> -        RW_ASSERT (0 == ((pagemask + 1) & pagemask));
> +        rw_fatal (0 == ((pagemask + 1) & pagemask),

let's go back to RW_ASSERT() here...

[...]
> @@ -286,7 +294,11 @@
>  _rw_table_grow ()
>  {
>      // table_max_size_ cannot be less than allocated blocks
> -    RW_ASSERT (table_max_size_ >= _rw_stats.blocks_);
> +    rw_fatal (table_max_size_ >= _rw_stats.blocks_,

...and here.

[...]
> -        RW_ASSERT (!"Invalid addr passed to the rw_free");
> +        rw_assert (0 == addr, __FILE__, __LINE__,

This should probably be an rw_error.

With these changes okay to commit.

Thanks!
Martin

Mime
View raw message