[boost] Re: smart assert library comments

From: John Torjo (john_at_[hidden])
Date: 2003-05-22 06:53:54

Hi Pavel,

Thanks for your comments.
Answers below.

> Hello John,
>
> I was looking over smart assert and it grew in size! I am not able to
> understand it immediatelly so I have some nitpicks now, usually related to
> style. After I grok it I'll send more.

Thanks. Indeed, it grew. I will try to create a summary of its features.
To start, you can now add thread-safety and persistence  ;-)
Will post probably tomorrow the update.

> smart_assert/smart_assert.hpp
>
> =============================
>
> 1. #include <stdlib.h> may be replaced with #include <cstdlib>
>
> However Dimkunware C++ library for MSVC 6 does put <cstdlib> functions
into
> std:: namespace, so

Unfortunately, it's not true (at least not for my MSVC6 ;-))

The following:

#include <cstdlib>
int main(int argc, char* argv[])
{
    std::abort();
 return 0;
}

generates a compile-time error on my VC6.

This is the sole purpose for #including <c*> files.

>
> 2. #include "boost/config.hpp"
>
> should be rather #include <boost/config.hpp>

True! Thanks. Done.

>
> 3. is #include <iostream> in smart_assert/smart_assert.hpp necessary?

I have to think about that.
Basically, I want to implement all the features, and then take off the
unnecessary headers.

>
> And where <iostream> is necessary maybe <iosfwd> would be enough.

True (see above).

>
> 4. the catch(...) may (under Windows) eat some system exceptions which
> should pass. It should generally be used only at top level of application.
>

I assume you're talking about Assert's destructor.

I did this in order to catch everything - to show to the programmer that the
logger functor (the functor that does the logging of the failed assertion)
should NOT throw.
Otherwise, this could go undetected.
Do you have a better suggestion?

>
>
> smart_assert/smart_assert_defs.hpp
>
> ==================================
>
> 1. the enum should use values 1 .. 255. This would allow (on some
platforms)
> implement it as char. Not that it is real performance helper but may keep
> some heart in peace.

At this time, I don't think this is so important. However, if others think
so, I can change them.

The point is that basically lvl_debug, lvl_warn etc. should be constant
integers. If you want to define your custom level, just define it as an
integer constant (or an unnamed enum, for that matter).

If you look at the code, the level is always treated as an 'int'eger.

>
> 2. is the <sstream> there necessary?
>
> 3. In addition to __FUNCTION_ GCC has __PRETTY_FUNCTION__ and C99 __func__
> (I think). Other platforms may have their own (like __function__).

Thanks!
Basically, I would be interested in all of these unportable macros.
The more the merrier, since the more information we can supply about a
failed assertion, the better!

If anyone has knowledge about these, please let me know. Thanks.

>
> This probably belongs to <boost/config.hpp> but I don't know if anyone
would
> dare to dig there.

I will try ;-) It's on my list.

>
>
>
> smart_assert/static_initializer.hpp
>
> ===================================
>
> - this is probably unfinished: there should be example

Indeed. There will be, after I finish it.
>
>
>
> smart_assert/smart_assert_context.hpp
>
> =====================================
>
> 1. the assert_context constructor should use initializer list:
>
> assert_context() : m_nLevel(lvl_debug) {}
>
> Its a matter of style.

Indeed. Done. Thanks.

>
>
>
> 2. function get_context_val() should return std::string() instead of "",
it
> is one conversion less and one string in executable less.

True. Done. Thanks. ;-)

>
> 3. I don't know if anyone may use wchar_t in assert (I personally cannot
> imagine why but maybe this can be also useful). Maybe for user messages?

I thought about this. But I really think it would complicate the code.
Anyway, if there are uses for it, I can have a compile-time switch, allowing
users to specify the assert_char_type.

>
> 4. the member template add_context_val<> can be also specialised for
> std::string parameter.
>
> I don't know if it matters for performance it may have specialisations for
> types like int, short to avoid passing as reference.
>
> Specialisation for char* and string would be also less performance hungry.
>
> A check for NULL pointer should be there (if this can happen - I don't
> know).

True. I can do it, but as I know, VC6 sometimes has problems with
specializing member function templates.
I added a FIXME though (possible improvement). Thanks.

>
> 5. Can struct val_and_str be replaced by std::pair<std::string,
std::string>
> ?

Yes they can. However, I thought it would be easier to understand and
manipulate like this - also, for readers of the code.
Do you agree?

(also, by looking at the code, you'll see that I avoided using std::pair;
it's only used once).

>
> 6. Maybe the m_aVals can be reserve()d to some value like 1,2, 4 to make
it
> faster in typical situation?

Indeed. Done ;-) Thanks.

>
> 7. the variables should have one line comment what they are for, their
names
> are quite short

True. Will do.

>
>
>
> smart_assert/smart_assert_util.hpp
>
> ==================================
>
> 1. the function get_typeof_level() may take enum (now unnamed) as
parameter,
> instead of int

No. I want the user to be able to define its own (integer) levels.

>
> 2. Namespace Private is opened, closed, opened and closed again.

I wanted to suggest that the get_typeof_level() function and the
log_context_val/log_val classes are not related to eachother.
But I guess it's not necessary. Removed it. Thanks.

>
> 3. doc for log_context_val and log_val is the same

I did not write too much about that, since they are only used internally.

>
> 4. in dump_all_assert_context() the
>
> << ":" << should be << '.' <<

Indeed ;-) Thanks.

>
> 5. maybe the context should keep file and line as separate items to
simplify
> the code - now it does a lot of activity to find them and I see some code
> taht can go away

This is a very interesting idea. I think it could be done.
I did not think too much about it, since analyizing the assert_context
should happen very rare (at each failed assertion - we should not have too
many of those ;-)).

Anyway, it's a possible improvement, and when I have the time for it, will
do it.

>
>
>
> smart_assert/smart_assert_loggers.hpp
>
> =====================================
>
> 1. the name should bot be hardcoded

True. Bronek Kozicki has pointed it out as well.
Will change it.

>
> 2. the file should not be opened until first assert happens.

Hmm... I have to think about that...
But you're probably right.

>
> 3. there may be different policies on file writing: overwrite/append.

True. But the idea is that you can set your own logger.
In there you can do whatever you wish (you're not even forced to output to a
file - for instance, you can use OutputDebugString for Win32).

Anyway, I will provide a function that will do just that (use
OutputDebugString).

>
> 4. some may sent logs to socket/event queue/pipe/whatever. Can this be
> somehow accomplished?

Yes. As said above, you can send it whereever you wish.
Just have a functor that takes one argument 'const assert_context &', and
from there, sky's the limit!

Now, your idea about keeping the file & line separately within the context
sounds even more appealing!

>
> 5. if file write fails, it should print at least to stderr or so

Yes, but what I do is call a functor (as said above) to do the logging.
Indeed, the default functor should do this (it does now - thanks!), but in
case you set your own logger functor, you'll have to do it yourself. I guess
that's pretty reasonable ;-)

>
> 6. Is time written into log file? Some may want precise timing (maybe
using
> Boost.Timer or Date)

Not now, but you can customize and do it.
It's actually very easy.
Something like:
#define SMART_ASSERT_CONTEXT
context("file",__FILE__).context("line",__LINE__).context("time",
some_conversion_function(time(NULL))

I guess I could include it by default. What do others think?

(#defining the SMART_ASSERT_CONTEXT should be done prior to #including
<boost/smart_assert.h>)

>
> 7. Is it possible to write into log info from applications directly? (good
> e.g. to compare timestamps later when assert happens)

I'm not sure I understood this, but I guess not.
Please provide me with an example.

>
> 8. Is it possible (as policy) write thread ID?

It can be do in different ways.
One way is to define SMART_ASSERT_CONTEXT

#define SMART_ASSERT_CONTEXT
context("file",__FILE__).context("line",__LINE__).context("threadid",some_th
read_id_function())

By the way, I wanted to ask William Kempf if there's a portable way to find
the current thread_id().

(#defining the SMART_ASSERT_CONTEXT should be done prior to #including
<boost/smart_assert.h>)

Another way is to have your own assert_base class, and add thread-ID to the
.context().
When I'll write the doc., I will explain about this.

Now that I come to think about this, the same can be done about the time()
(point 6.)

>
> 9. (I am starting to fantasize now) Would it be possible to define some
> global callback to write some data into log when assert happens? Could
there
> be 'stack' of these callbacks?

;-)))

But this is what the logger is for ;-)
The logger loggs ALL assertions.
Your logger can be implemented as a functor. For instance, an object, in
which you can add/remove streams. When a message is written to the logger,
the logger can then (in our example) write the context to all its streams.

Or did I not understand what you meant?

Note that there's a BIG difference between the logger and the handler of an
ASSERTion.
The logger logs the assertion (no matter its level), and the handler handles
it. You can, for instance, have a handler that ignores all assertions (but
they are logged anyway - by the logger). Do you make any sense of what I
just said ;-)) ?

>
>
>
> smart_assert/smart_assert_keeper.hpp
>
> ====================================
>
> 1. it should be possible NOT to define v_ or define it with diffenent
name.
>

True. I thought about this. You're totally right.
I now have a directive that disables it. Thanks.

> 2. the check for Windows should use be like in Boost config. The __asm
would
> work on MSVC, Intel, BCB for what I know, I am not sure about GCC.
>
> Also user may offer their own break-into-debugger routine even when int 3
is
> available.

Indeed, I still have to see how to do this. Any help is appreciated.
Basically, I want to know whether the processor is Intel (in this case, 'int
3' works).

(note: That's why I have the BOOST_BREAK_INTO_DEBUGGER_DEFINED directive -
so that the user can define its own function).

>
> 3. Also on projects with exceptions switched off (BOOST_NO_EXCEPTION
> defined) some code should be commented out.
>

True. Thanks! I've done it! Hhew ;-)

> 4. <crazy mode on>
>
> what if I want to send assert to some remote machine in distributed
system,
> is there a chance to overtake the assert output completely _and_ send it
> somewhere with TIMEOUT PROTECTION?

Sorry, but I don't understand what you meant.
But I guess it's possible ;-))))

>
> </crazy mode off>
>
>
>
> smart_assert/smart_assert_verify.hpp
>
> ====================================
>
> 1. Maybe the SMART_VERIFY can be documented in detail - I was looking on
it
> and wan not able to orient what the code does.

It will be ;-)
It's just like SMART_ASSERT, only that it works in release as well.

>
> 2. the namespaces there are not needed

True. Are there just for consistency. I might put something there later ;-)

>
> 4. Note to naming:
>
> Maybe BOOST_SMART_ASSERT and BOOST_SMART_VERIFY should be used by default
> and users can redefine them as they need.
>

True. However, someone suggested that since the library could (<crazy mode
on>) make it into STL, use SMART_ASSERT/SMART_VERIFY (<crazy mode off>).

However, I can change their names - it's just a few changes in the code.

>
>
> General thoughts:
>
> =================
>
> 1.imagine situation: the system is corrupted, nothing works and now the
> assert would do a lot - allocate memory, create complex structures etc.
>
> - there should be protection agains std::bad_alloc

Basically, from Assert, I protect against all exceptions.
See that the all the code is surrounded in try/catch.
Also, every parameter is taken by reference, so that no exception will occur
during copy-construction.

If an exception occurs inside a method from Assert, I'll catch it and
set_ignore_this_assert(). This way, this assert will be ignored.

I don't know if I can do more than that ;-)

>
> - there should be protection against system exceptions happening within
> assert
>
> Maybe if it is not possible to do 'normal' assert activity some emergency
> printout to console and abort() should be done - something really really
> basic and crashproof as fallback

I have to think about that. But I guess it would complicate things a lot.

>
> 2. is it possible to pass assert context info (original structure, not
> flattened into stream) into an thrown exception?

Yes, why not?
You can set your own 'error' handler (not the default), something like this:

    inline void default_handle_error( const assert_context & context) {
        throw assert_context_exception( context);
    }

>
> 3. would it be possible somehow to say: allocate enought memory for assert
> internal structures beforehand so std::bad_alloc cannot happen during
> asserts?

I was thinking about that myself.
But I wanted to do it for the second version. What do you all think?

>
> 4. could there be a callback function to be called before and function to
be
> called after assert? E.g. I may want to pause and resume all other threads
> before/after assert. I think something with templates may have zero
> overhead. It may also find use in application with many processes (take
> snapshot or so).

Well, I guess so.
But I think you could do this within your own (custom) assert handler.
Are thinking of something global (unlike the assert handlers, an assert
handler being related to a level)?

>
> 5. thought: what about calling assert() (the _original_ one from assert.h)
> instead of abort() when it is abailable (no NDEBUG)? You may get platform
> specific UI and behavior. When assert() is not present, then fall back to
> abort().

I don't know how useful would that be.
For instance, for Win32, the platform specifiy UI and behavior is within
abort() itself. I'm not sure it would be necessary.

>
> This can work even when user redefines SMART_ASSERT as assert.

Sorry, what do you mean?

>
> 6. last but not least: what is the impact of SMART_ASSERT (I mean in debug
> mode) on compile time/code size/memory fragmentation/etc? For example I
use
> assert often, very often and am sensitive on this.

compile time - it will take a little more to compile. However, once I
implement all features, I will do my best to optimize this.

code size - it will be a little more than regular assert(), but not with
much, even though all the code is inline.

memory fragmentation - I really don't know.

Thanks so much for your comments.

Best,
John