Re: Debug algorithms

[Jonathan Wakely <jwakely at redhat dot com>]

On 06/09/16 22:26 +0200, François Dumont wrote:
> Hi
>
>    Any final decision regarding this patch ?
>
>    Note that __std_a namespace is optional, I can remove this change 
> from the patch if you want.

Can you describe what difference that would make? Would it avoid all
the changes to Parallel Mode? Given that Parallel Mode is hardly used
by anyone, and will be supersedeed by the C++17 parallel algorithms, I
don't think we should be spending much time making huge changes to it.

The patch is very large, but a lot of it seems to have nothing to do
with the actual change e.g.

-  template<typename _RAIter, typename _Compare>
+  template<typename _RAIter, typename _Comp>
    void
-    sort(_RAIter, _RAIter, _Compare);
+    sort(_RAIter, _RAIter, _Comp);

This seems to be some unrelated cleanup that just makes the patch
larger.

The __std_a namespace is just used for indirection to refer to either
std::foo or std::_GLIBCXX_STD_A::foo, is that right? So no function
templates in normal mode change mangled name, because they're still in
namespace std?

This is a pre-existing problem, but I hate the fact that depending on
the mode, _GLIBCXX_STD_A is either the top-level namespace, or a
nested one. That means it can't be used to form a fully-qualified
name. e.g. _GLIBCXX_STD_A::copy is either std::copy or
__cxx1998::copy. Why isn't the latter std::__cxx1998::copy instead?
It would be cleaner if ::_GLIBCXX_STD_A::copy was valid, i.e. the
macro expands to std or std::__cxx1998. For comparison,
<tr1/gamma.tcc> does this:

 #if _GLIBCXX_USE_STD_SPEC_FUNCS
 # define _GLIBCXX_MATH_NS ::std
 #elif defined(_GLIBCXX_TR1_CMATH)
 namespace tr1
 {
 # define _GLIBCXX_MATH_NS ::std::tr1

This means _GLIBCXX_MATH_NS::foo is always valid in any scope.


It's not clear to me from reading the patch when I should use __std_a
and when I should use std. If I forget to use __std_a (and I *will*
forget, and other people probably will too) what happens? We get
slightly slower code in Debug Mode? Anything else?

But if you get rid of __std_a then maybe the questions above don't
need to be answered anyway.


Some other feedback:

I don't like the name __cxx1998_a. The __cxx1998 name is historical,
but it contains C++11 features too. For a new namespace I'd prefer a
name that isn't misleading. The "cxx1998" part is untrue, and the
important part is "a" which doesn't tell you anything.

Does the earlier unwrapping of debug iterators lose any safety, or
will all the same checks always get run?  e.g. if you call an algo in
debug mode today and it increments past the end then the iterator
aborts. If we unwrap the debug iterator and invoke a normal algo,
will we lose that check? Assuming the debug algo uses
__glibcc_require_valid_range() we should be OK, but I'd like to be
sure.

How much performance does this actually buy us? I don't really mind if
we can't make Debug Mode faster than it is now. We have the
lightweight assertions, which don't change algorithmic complexity and
don't change ABI. IMHO it's OK if Debug Mode is heavier and slower.

We need to get better at documenting things. The interaction between
the different namespaces in Debug Mode, Parallel Mode etc is already
confusing, and I think this patch would make it worse. The namespaces
used for the math special functions should be documented too (but it's
simpler there, we just have std and std::tr1, with nested __detail).
Is all the information in the manual still correct at
https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_design.html ?



> François
>
>
> On 18/07/2016 21:25, François Dumont wrote:
> > On 13/07/2016 19:45, Jonathan Wakely wrote:
> > > On 22/06/16 22:05 +0200, François Dumont wrote:
> > > > Hi
> > > >
> > > >   Here is eventually the so long promized patch to introduce 
> > > > Debug algos similarly to Debug containers.
> > >
> > > I'm trying to decide how much benefit this really gives us, and
> > > whether the obfuscation to the code (even more namespaces involved,
> > > and having to use __std_a:: instead of std::) is worth it. It also
> > > means more code to maintain of course, with extra overloads.
> > >
> > > >   Why such an evolution:
> > > > - More flexibility, debug algos can be used explicitely without 
> > > > activating Debug mode.
> > >
> > > Although nice in theory, I doubt this will get much usage in practice.
> > >
> > > > - Performance: Debug algos can get rid of Debug layer on top of 
> > > > container iterators to invoke normal algos. Operations on normal 
> > > > iterators are faster and we also benefit from the same algos 
> > > > specialization that sometimes exist on some container iterators 
> > > > (like std::deque ones). Also normal algos are now using other 
> > > > normal algos, Debug check won't be done several times.
> > > > - It will be easier to implement new Debug checks without the 
> > > > limitation to do so through some Debug macro
> > > >
> > > >   To do so I introduced a new namespace __cxx1998_a used for 
> > > > normal algos when Debug mode is active. I couldn't reuse 
> > > > __cxx1998 cause with current implementation of Debug containers 
> > > > __cxx1998 is exposed and because of ADL we could then have 
> > > > ambiguity between Debug and normal versions of the same algos. I 
> > > > also introduced a __std_a namespace which control the kind of 
> > > > algos used within the library mostly for containers 
> > > > implementation details.
> > >
> > >
> > > I think I need to apply the patch locally and spend some time looking
> > > at the new structure, to see what ends up calling what. I'm finding it
> > > difficult to follow that just from reading the patch.
> > This is definitely more code to maintain but I hope this code won't 
> > require much maintenance as it only host the Debug logic and not the 
> > algo logic itself. It relies on normal algo for algo logic.
> >
> > You can see this patch as a way to cleanup the normal mode too !
> >
> > If __cxx1998 namespace was perfectly encapsulated we could avoid the 
> > __cxx1998_a but for the moment the boundary between normal and debug 
> > mode is too tight.
> >
> > François