Boost C++ Libraries: Ticket #7730: Generic specializations of is_nullary for custom terminals are not possible

Tue, 04 Feb 2014 00:06:52 GMT

I have implemented your test and am putting it onto develop.

I have set it up so that for the moment it is necessary to do this before the header to use this.

#define BOOST_PHOENIX_SPECIALIZE_CUSTOM_TERMINAL

There is a test bug7730 which will test the header with the define.

Please let me know if this works for your case.

Tue, 04 Feb 2014 00:10:58 GMT

Replying to andysem:

The is_nullary trait has a second template parameter which is intended to be used exactly for this purpose. A nested tag void typedef can be used to match the trait for a set of types. I could create the following specialization:
template <typename T>
struct is_nullary<custom_terminal<T>, typename T::_is_my_terminal >
    : mpl::false_
{};

Note: I have not implemented this part of your example because as I read it this part will go into your client code. Is that correct?

Andrey Semashev — Tue, 04 Feb 2014 02:42:39 GMT

Replying to John Fletcher <J.P.Fletcher@…>:

I have implemented your test and am putting it onto develop.

I have set it up so that for the moment it is necessary to do this before the header to use this.
#define BOOST_PHOENIX_SPECIALIZE_CUSTOM_TERMINAL
There is a test bug7730 which will test the header with the define.

Please let me know if this works for your case.

Since this bug affects Boost.Log (a library), I cannot define the macro in my code, so it doesn't really help me. In order to work, this has to be the default (and the only) behavior of Boost.Phoenix. To my understanding, it should be safe enough to enable it by default the way I described in the ticket, although, of course, I may be missing something.

Replying to John Fletcher <J.P.Fletcher@…>:

Note: I have not implemented this part of your example because as I read it this part will go into your client code. Is that correct?

Yes.

Tue, 04 Feb 2014 07:10:19 GMT

O.K. I will leave it on test today to make sure that it is O.K. with other compilers. If all is well I will move it to the default operation then. I will notify that here.

You may realise that I have only recently taken on the maintenance of phoenix and I don't want to break things by moving too fast. I want to get some good patches and then move them from develop to master in time for the next release of boost.

I am concerned that there are a number of implications for the log library from problems in boost. Please let me know what the problems are and whether some of the fixes I have done have helped. Also what the most urgent things are to be fixed next.

Andrey Semashev — Tue, 04 Feb 2014 07:35:13 GMT

Replying to John Fletcher <J.P.Fletcher@…>:

O.K. I will leave it on test today to make sure that it is O.K. with other compilers. If all is well I will move it to the default operation then. I will notify that here.

You may realise that I have only recently taken on the maintenance of phoenix and I don't want to break things by moving too fast. I want to get some good patches and then move them from develop to master in time for the next release of boost.

Ok, no worries. It's not a fatal issue since the specializations for Boost.Log-specific terminals do their job of covering this problem. I appreciate your effort of taking care of Boost.Phoenix, thanks a lot for that.

I am concerned that there are a number of implications for the log library from problems in boost. Please let me know what the problems are and whether some of the fixes I have done have helped. Also what the most urgent things are to be fixed next.

I think I've created tickets as I discovered problems or was told by Boost.Log users. You can find them by filtering those reported by me. I think the most critical ones right now are #7996 and #9363.

Tue, 04 Feb 2014 21:27:36 GMT

I tried switching the patch in this to be default on and it breaks other headers unfortunately. I need to investigate further. John

Tue, 04 Feb 2014 23:25:33 GMT

Replying to John Fletcher <J.P.Fletcher@…>:

I tried switching the patch in this to be default on and it breaks other headers unfortunately. I need to investigate further. John

This is not working.

Thu, 06 Feb 2014 23:52:46 GMT

I have now sorted out the problems and committed a proposed solution to develop. Test bug7730 tests turning it off by defining BOOST_PHOENIX_NO_SPECIALIZE_CUSTOM_TERMINAL. John

Fri, 21 Feb 2014 07:49:15 GMT

WARNING - this fix may break client code which uses the boost phoenix custom_terminal.

The solution may well be to insert the following into any overload of the struct boost::phoenix::custom_terminal:

#ifndef BOOST_PHOENIX_NO_SPECIALIZE_CUSTOM_TERMINAL

typedef void _is_default_custom_terminal; fix for #7730

#endif

John

Fri, 21 Feb 2014 09:40:18 GMT

Replying to John Fletcher <J.P.Fletcher@…>:

WARNING - this fix may break client code which uses the boost phoenix custom_terminal.

The solution may well be to insert the following into any overload of the struct boost::phoenix::custom_terminal:

John

The code should be in a box:

#ifndef BOOST_PHOENIX_NO_SPECIALIZE_CUSTOM_TERMINAL
       typedef void _is_default_custom_terminal; // fix for #7730
#endif

Andrey Semashev — Sat, 22 Feb 2014 06:51:43 GMT

Can this typedef be added to the default custom_terminal somehow? I suspect breaking the client's code this way won't be received well.

Sat, 22 Feb 2014 09:15:52 GMT

The typedef is provided in the default custom_terminal - see the code below from

boost/phoenix/core/terminal.hpp

    template <typename T, typename Dummy>
    struct custom_terminal
#ifndef BOOST_PHOENIX_NO_SPECIALIZE_CUSTOM_TERMINAL
    {
      typedef void _is_default_custom_terminal; // fix for #7730
    }
#endif
      ;

The problem arises when people developing their own terminal specialize the custom terminal. The case which was made in this bug report was for them to have the freedom I have now given. It has needed a fix in the case of Boost Spirit. I have now included a check on the spirit headers in the testing of phoenix, to pick up anything similar before it even gets into develop.

Sat, 22 Feb 2014 09:47:46 GMT

I believe I have provided a solution to this problem and also provided the user with a way to have the previous behaviour by defining BOOST_PHOENIX_NO_SPECIALIZE_CUSTOM_TERMINAL.

I am now being criticised for defining such things.

This kind of switches make it difficult to use together different libraries which themselves use phoenix, unless the macro is dragged and honored to every piece of code defining a custom terminal.

I don't believe that is true. You don't need to use it unless you need the old behaviour. Because all the tests for it are negative it does not have to be defined at all.

I want to resolve this and close this bug as soon as I can. John

I thought that was a good thing to do but could remove it and force the new behaviour.

Andrey Semashev — Sat, 22 Feb 2014 10:58:13 GMT

There is another problem with Boost.Log, see #9693. I'm not sure what is the best course of action with that problem as I don't quite understand what is the problem there. My original suggestion in this ticket implied that the existing code would not break after this change, but I'll understand if something needs to be done in Boost.Log. My main concern though is that this can affect other Boost users (i.e. outside Boost). Is that intentional and can it be avoided (without the config macro, I mean)?

Don't get me wrong, I'm not criticizing you, and I want this change work eventually.

Sat, 22 Feb 2014 11:15:27 GMT

Andy

The only code which will break with this change, as far as I can see, is code which has a specialization of the custom terminal. I have just fixed that for Boost Spirit. The only change needed was to add the typedef. I don't think there are going to be many of those around, but I have modified the entry in ChangeLog to be clearer.

I intend to make the documentation clear as I get into that side of things.

I have come to the conclusion that the client specialization does not need to use the BOOST_PHOENIX_NO_SPECIALIZE_CUSTOM_TERMINAL as the extra typedef is all there is. I intend to check that out, as I understand the reluctance of maintainers of other libraries to have such things in their code.

I will look into #9693 when I get a chance. I have decided to port the proto tool display_expr inot phoenix to have a diagnostic tool. I have the basics working but need to do more on the tags to get sensible output. When I have that I will be able to dig into phoenix better. John

Andrey Semashev — Sat, 22 Feb 2014 11:26:48 GMT

Nevermind the #9693, I found out that I forgot to specialize is_nullary for my terminal. I wonder how it worked before.

Regarding the macro. I kind of agree with the quote you presented in comment 13 because now in Boost.Log I apparently have to support both configurations. I.e. the old code with multiple is_nullary specialization don't disappear but potentially is made conditional, which is worse. Can this macro be removed and the new code be always in action?

Sat, 22 Feb 2014 11:45:02 GMT

I have just attempted to reply and it has been rejected as spam. Would you please email me outside this system? John (J.P.Fletcher@…)

Sat, 22 Feb 2014 12:15:56 GMT

What is needed (I think) is to provide a default value for the second template parameter. At the moment the default is void, so could we catch that? Seems risky to me. John

Andrey Semashev — Sat, 22 Feb 2014 13:13:35 GMT

Replying to John Fletcher <J.P.Fletcher@…>:

What is needed (I think) is to provide a default value for the second template parameter. At the moment the default is void, so could we catch that? Seems risky to me. John

Not sure what you mean, but I don't think the actual type matters. That parameter just allows to use SFINAE to eliminate specializations instead of the standard template specialization mechanism. Void is just something neutral and convenient enough.

You could probably create some other mechanism to detect the default custom_terminal specialization. But that would still require some kind of type introspection, I think, and would arguably be slower in compile time.

What I was suggesting is not change the approach with the _is_default_custom_terminal typedef, but just make it unconditional. You fixed Boost.Spirit already and as you said the problem is unlikely to appear outside Boost. So just document the needed change for those few who encounters the problem.

Sat, 22 Feb 2014 13:49:11 GMT

That is the default behaviour at the moment, so you would have to put typedefs in the ones you define. Is that sufficient or do you need more? If it is, all I need to do is to get rid of the conditional code. John

Andrey Semashev — Sat, 22 Feb 2014 14:22:53 GMT

Replying to John Fletcher <J.P.Fletcher@…>:

That is the default behaviour at the moment, so you would have to put typedefs in the ones you define. Is that sufficient or do you need more?

Yes, it is sufficient.

Thomas Heller — Tue, 11 Mar 2014 23:03:27 GMT

I don't see why such a fix is necessary. Wouldn't this work?

template <typename T>
struct is_nullary<custom_terminal<even_more_custom<T> > : ...

This wouldn't break any exisiting code.

FWIW, The dummy parameter only has the side effect of being able to be used with SFINAE. The main intention was to get rid of some uneeded template instatiations.

Andrey Semashev — Wed, 12 Mar 2014 06:51:51 GMT

Replying to theller:

I don't see why such a fix is necessary. Wouldn't this work?
template <typename T>
struct is_nullary<custom_terminal<even_more_custom<T> > : ...
This wouldn't break any exisiting code.

This is what I have to do currently. The problem is that I have many such even_more_custom terminals and I have to specialize is_nullary for each and every one of them.

FWIW, The dummy parameter only has the side effect of being able to be used with SFINAE. The main intention was to get rid of some uneeded template instatiations.

You can't use the parameter for SFINAE now because of the generic specialization I pointed out in the ticket description.