Re: [boost] Formal Review: any

From: Kevlin Henney (kevlin_at_[hidden])
Date: 2000-11-07 12:11:11

In message <00d901c04891$a5e85b20$0200a8c0_at_yogi>, Mark Rodgers
<mark.rodgers_at_[hidden]> writes
>Documentation:
>
>- Needs boost header.
>
>- The link to lexical_cast will need to come out until 
>  lexical_cast is accepted into Boost.
>
>- The link to any.hpp at the top of the Synopsis would be broken 
>  if any.hpp lives in boost/, which is where it should indeed 
>  live.

I planned to sort these out at such time it is actually accepted into
the library.

>- I find the documentation of the members very confusing.  The
>  member signatures are indented and not in bold, so they look 
>  like code snippets embedded in the text rather than headings.

That would be the same style as the standard :->

If there's a strong enough feeling, I can go for making the elements
bold.

>any.hpp
>
>- It might be nice to at least be able to compare anys for
>  equality - equal if same type and contents equal.  An ordering
>  using type_info::before might even be possible.  On the other
>  hand, that puts additional constraints on ValueTypes.  Not
>  sure what's best.  As it is, the void* conversion probably
>  allows comparisons with somewhat surprising results.

Requiring equality comparison is too strong for what any is. Using
type_info ordering also creates surprising results :-}

>- The conversion to void* is IMHO A Bad Thing.  Please remove it
>  and replace with 
>
>     bool empty() const { return content == 0; }

A matter of taste that I can appreciate, but I'm reasonably happy with
the usage based on the UDC.

>- I wonder why swap returns *this.  The normal convention is for
>  swap functions to return void.

Force of habit: I prefer chainable results to dead ends. If there is a
strong feeling about this I can replace it with void.

>- I think you need to either specialise std::swap or provide a
>  swap function in boost::, depending on your preference.  I think
>  the jury is out on what is the correct thing to do for non-
>  templates (I think probably the former).

Agreed. A full specialization in std seems to be the preferred approach.

>- There are two distinct bugs being worked around with MSVC_INCLUDE
>  and MSVC_EXCLUDE.  GCC shares one of them, so it is not possible
>  to make any work for GCC just by defining COMPILING_MSVC (which
>  you should anyway integrate into config.hpp as noted in future
>  directions).  By commenting out the MSVC_INCLUDE on the copy
>  constructor and assignment operator, and by defining COMPILING_MSVC
>  since GCC shares the inability to write object.memfun<foo>(), I
>  was able to make this work with GCC. 

It's now easier to generalise the bug feature set, so feature specific
macros will be provided and placed in config.hpp.

However, g++ does not quite share one of the bugs: it is legal to
qualify use of to_ptr by prefixing it with the template keyword. This
would be the preferred approach.

>- I wonder if it would be a good idea to provide a pair of to_ptr
>  functions for MSVC's and GCC's benefit?
>
>    template <typename ValueType>
>    ValueType *to_ptr(any &a)
>      { return a.to_ptr MSVC_EXCLUDE(<ValueType>)(
>                MSVC_INCLUDE(static_cast<ValueType *>(0))); }

This looks like another 'global' cast operator. In which case it should
probably follow more conventional naming. However, I'm not sure how much
we want to pollute the namespace with the different ways of getting
values out given that the same functionality is already possible in
three different forms on most compilers, and two on MSVC.

>  and similar for const?
>
>      boost::any a(23);
>      int *p = to_ptr(a);
>
>  seems a lot nicer than
>
>      boost::any a(23);
>      int *p = a.to_ptr(static_cast<int *>(0));

The second one is not supported for MSVC (and will not be supported for
g++). Sure, it will compile, but the documentation is clear that it is
not supported.

>test-any.cpp:
>
>- Please change 
>    #include "any.hpp" 
>  to 
>    #include "boost/any.hpp"

Yup, if and when the library is accepted, the final boostifying touches
will be added.

>- The test_comparisons function is very misleading since it gives
>  the impression that anys can be compared sensibly.  Comparing
>  0 < &typeid(void) seems particularly unuseful.

Results in true, in exactly the same way that false < true does.

>test.hpp:
>
>- Needs copyright notice.
>
>- I agree with Jeremy - using string as a template parameter is
>  confusing.

Yes and yes.

Kevlin
____________________________________________________________

  Kevlin Henney                   phone:  [phone]
  Curbralan Limited               mobile: [phone]
  mailto:kevlin_at_[hidden]     fax:    [phone]
  http://www.curbralan.com
____________________________________________________________