Re: [patch] debug mode

[Paolo Carlini <paolo dot carlini at oracle dot com>]

Hi,

a few comments before my vacation days. In general the new patch looks
pretty good, some comments below.
> Hello
>
>     Here is my new patch proposition:
>
> - I have tested it on x86_64 linux with make check. make debug-check
> gives me the same failures with or without the patch, the failures
> looks normal ones to me. For instance the synopsis.cc tests are all
> failing because of ambiguity between std::vector and
> std::__debug::vector, other tests are failing because of tests
> involving the profile mode.
Well, before your patch only the synopsis.cc testcases are failing,
right? Please make sure we don't regress about that, even by way of
undefs, or whatever other trick, thanks.
> - 80 columns
>
>     Sorry about that I looked for the number of chars and not the
> number of columns, because of tabulations it is not equal. By the way,
> are tabulations ok ? Most of projects I work on was forbidding tabs,
> should I consider that tabs are 8 spaces long ? To fix this problem I
> replace many __first, __last by respectively __f, __l, I hope it is fine.
Certainly tabs are not forbidden, but the indentations must be right, 2
chars wide each, essentially. About __first and __last, I would suggest
not changing those to __f and __l if at all possible: short names are
always more dangerous vs BADNAMES on some less widespread machines.
> - Performance gain
>
>     This patch contains 4 new performance tests to challenge range
> constructors of different containers instanciated with int type:
>     deque_construct: deque instances built from a vector [begin, end[
> range
>     list_construct1: list instances built from a vector [begin, end[
> range
>     list_construct2: list instances built from a list [begin, end[ range
>     vector_construct: vector instances built from a vector [begin,
> end[ range
>
>     When run in release mode it gives the following results:
> deque_construct.cc                                               
> 11r    9u    1s        0mem    0pf
> list_construct1.cc                                               343r 
> 341u    0s        0mem    0pf
> list_construct2.cc                                               346r 
> 346u    0s        0mem    0pf
> vector_construct.cc                                               
> 8r    2u    5s        0mem    0pf
>
> deque and vector build are much faster because thanks to internal
> optimizations it relies on memmove.
>
>     Once run with _GLIBCXX_DEBUG defined without the patch it gives:
>
> deque_construct.cc                                               529r 
> 523u    2s        0mem    0pf
> list_construct1.cc                                               895r 
> 890u    1s        0mem    0pf
> list_construct2.cc                                               884r 
> 879u    1s        0mem    0pf
> vector_construct.cc                                              475r 
> 467u    6s        0mem    0pf
>
> All tests are taking much more time. list_construct1 and
> list_construct2 because we need more time to check safe iterators on
> each initialization, deque_construct and vector_construct because
> usage of safe iterators disable completely usage of memmove and so
> once again we have to check each safe iterators each time we increment
> the safe iterator.
>
>     With the proposed patch we have:
>
> deque_construct.cc                                               
> 11r    7u    2s        0mem    0pf
> list_construct1.cc                                               341r 
> 339u    0s        0mem    0pf
> list_construct2.cc                                               865r 
> 861u    1s        0mem    0pf
> vector_construct.cc                                               
> 9r    3u    4s        0mem    0pf
>
>     As soon as we detect random access safe iterator we check the
> whole range in a single operation and there is no more overhead on
> each iterator incrementation. Moreover for deque construct and vector
> construct we restore usage of memmove. Only list_construct2 is not
> impacted because we are not able to validate the iterator range in a
> single operation so we still have the overhead of checking it each
> time we increment the safe iterator internally.
Looks good. But if we are talking about performance, those tests belong
to testsuite/performance and should use the timing infrastructure
already in place.
> - Modifications of functions.h
>
>     The only point that I am not fully comfortable with is that I had
> to introduce a declaration of __check_singular in formatter.h in order
> to have functions.h to depend on formatter.h and not the opposite as
> it used to be when formatter.h was including debug.h. If you do not
> like it I can introduce a new header to define __check_singular and
> associated __check_singular_aux that will be overloaded in
> safe_iterator.h. Except that I think modifications are correct and as
> a matter of fact the test suite did not show any problem.
Ok, let's give that a bit more testing (maybe normal make-check too, but
*without* PCHs? Normally I do that when I change the header dependencies).

Also, I see various testcases defining explicitly _GLIBCXX_DEBUG:
normally we don't do that: check-debug takes care of testing the
debug-mode behavior, uniformly. Please see if you can remove those
explicit defines in the testcases.

Finally, the *cont_traits.h headers should also use a correct style, no
open bracket at the end of a line, I would also recommend a minimum of
"air" between the lines, and make up your mind, either you uglify (eg,
_It), or not (eg, first). I would say not, for testcases.

Paolo.