Re: [boost] [optional] New RC_1_34_0 regression

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2007-04-05 18:44:57

Hi Thomas,

> Please go ahead. It's your call whether to simply revert or accept
> Richard's patch. The way I look at it is that the patch has higher
> risk
> but a nice testcase, that makes it even to me.
>
Well, it turns out that reverting isn't as straightforward as I figured.
The problem is that the previous version used two separate headers 
"none_t.hpp" and "none.hpp", with only the first being included by 
"optional.hpp", forcing users to include "none.hpp" manually.
That separation was simply intended to allow Borland users to work around 
the PCH problem. But only so much.

A nice feature of the "new" implementation (the one that caused all this 
trouble) is that you no longer need to include "none.hpp" manually anymore 
if you use optional<>.

But I really hated the idea of going back to 2 headers, so I spent quite 
some time trying to make an informed decision.

First, I searched the list archives for threads about 'none'. Interestingly 
enough, I found this:

http://thread.gmane.org/gmane.comp.lib.boost.devel/146009/focus=146126

There you can see the proposal for the alternative implementation, which 
eventually I put in place and casused all this, but, you can also see that 
at _that_ time, I was much much more lucid that in the recent days and 
figured that it would cause the issue Richard just found.
Furthermore, in that thread there is a different implementation proposed by 
Anthony Williams which, incidentally, is the same Steven just posted here 
(that code is slightly more elaborated, but the principle is the same).
You can also read that I agreed at the time with Anthony's implementation 
(...but then my clone forgot about it and picked the wrong one to commit ;)

Then I tried out Anthony's version (attached here) with the Optional 
testsuite but also with a new specific test (derived from Richard's post 
here, see attached).

The tests pass with toolsets msvc-8.00 and msvc-7.1 in WinXP.
I can test it with gcc (4.4.1) in a Debian Linux Box, but not today (8pm 
already here). And probably not until next Monday.

I installed the command line version of borland 5.5.1, but when I run 
"bjam --toolset=borland", bjam crashes, so I couldn't test there.

Can those participating in this thread take a look at the new 
implementation, test if you can, and report back so we can decide if it is 
safe to commit?

TIA

=======================================================================
// Copyright (C) 2003, Fernando Luis Cacciola Carballal.
// Copyright (C) 2007, Anthony Williams
//
// Distributed under the Boost Software License, Version 1.0.
// (See accompanying file LICENSE_1_0.txt or copy at
// http://www.boost.org/LICENSE_1_0.txt)
//
// See http://www.boost.org/lib/optional/ for documentation.
//
// You are welcome to contact the author at:
// fernando.cacciola_at_[hidden]
//
#ifndef BOOST_NONE_17SEP2003_HPP
#define BOOST_NONE_17SEP2003_HPP

namespace boost
{
  namespace detail
  {
    class none_helper;
  }

  inline void none(detail::none_helper);

  namespace detail
  {
    class none_helper
    {
    private:

      none_helper( none_helper const& ) {}

      friend void boost::none(none_helper);
    };
  }

  typedef void (*none_t)(detail::none_helper);

  inline void none(detail::none_helper) {}
}

#endif

none_test.cpp:
=======================================
#include <boost/optional/optional.hpp>
// Test that none.hpp is included with <optional.hpp>

// Left undefined to cause a linker error if this overload is incorrectly 
selected.
void verify_no_implicit_conversion_to_int ( int i ) ;

void verify_no_implicit_conversion_to_int ( boost::optional<int> const& ) {}

int main()
{
  verify_no_implicit_conversion_to_int( boost::none );
}

=======================================