Re: [boost] Header ordering [was: Vector of Polymorphic Pointers]
Thorsten Ottosen <tottosen@dezide.com> writes:
Robert Ramey wrote:
Note that its not enough to include BOOST_CLASS_EXPORT. It has to be included after the *archive.hpp classes that you're using. Double check that this is the case.
Robert, why is this needed? It seems like something that is *very* easy to forget and get wrong.
Yes, header ordering is notoriously hard to control. Robert, I'd like to discuss the design problem it is solving and see if we can come up with a better approach. Can you boil it down to a really simple example? -- Dave Abrahams Boost Consulting www.boost-consulting.com
This is explained in the docmentation under Reference/Special Considerations/Exporting Class Serialization Robert Ramey David Abrahams wrote:
Thorsten Ottosen <tottosen@dezide.com> writes:
Robert Ramey wrote:
Note that its not enough to include BOOST_CLASS_EXPORT. It has to be included after the *archive.hpp classes that you're using. Double check that this is the case.
Robert, why is this needed? It seems like something that is *very* easy to forget and get wrong.
Yes, header ordering is notoriously hard to control. Robert, I'd like to discuss the design problem it is solving and see if we can come up with a better approach. Can you boil it down to a really simple example?
"Robert Ramey" <ramey@rrsd.com> writes:
This is explained in the docmentation under
Reference/Special Considerations/Exporting Class Serialization
Robert Ramey
I have analyzed your issue and I believe there is a better way; one that avoids the header ordering requirement. I have put together a small demonstration, which is enclosed. This shows that we can instantiate a class template (instantiator) for the exported class and each archive that's been seen. Then the only challenge is causing that class template instantiation to actually instantiate executable code. Fortunately, I've recently addressed that problem in changes to the serialization library. The basics are in CVS at: boost/boost/concept/detail/general.hpp boost/boost/concept/detail/msvc.hpp boost/boost/concept/detail/borland.hpp (Ignore the part that says BOOST_OLD_CONCEPT_SUPPORT), so we have the technology. And if you use the function pointer technique of BOOST_CONCEPT_ASSERT(( ... )) in boost/boost/concept/assert.hpp you'll be able to use BOOST_CLASS_EXPORT on class template instances with multiple arguments. BOOST_CLASS_EXPORT(foo<bar,baz>) // an error, today. I hope all this is helpful. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams <dave@boost-consulting.com> writes:
"Robert Ramey" <ramey@rrsd.com> writes:
This is explained in the docmentation under
Reference/Special Considerations/Exporting Class Serialization
Robert Ramey
I have analyzed your issue and I believe there is a better way; one that avoids the header ordering requirement. I have put together a small demonstration, which is enclosed. This shows that we can instantiate a class template (instantiator) for the exported class and each archive that's been seen.
Then the only challenge is causing that class template instantiation to actually instantiate executable code. Fortunately, I've recently addressed that problem in changes to the serialization library. The basics are in CVS at:
boost/boost/concept/detail/general.hpp boost/boost/concept/detail/msvc.hpp boost/boost/concept/detail/borland.hpp
(Ignore the part that says BOOST_OLD_CONCEPT_SUPPORT), so we have the technology. And if you use the function pointer technique of BOOST_CONCEPT_ASSERT(( ... )) in boost/boost/concept/assert.hpp you'll be able to use BOOST_CLASS_EXPORT on class template instances with multiple arguments.
BOOST_CLASS_EXPORT(foo<bar,baz>) // an error, today.
I hope all this is helpful.
Sorry, the attachment came with the wrong MIME type; I don't know if that's a problem for anyone, but here it is again: -- Dave Abrahams Boost Consulting www.boost-consulting.com
I've given a quick look at your post and attachtment. I vividly recall the making several attempts to get BOOST...EXPORT to compile - then to instantiate code. In fact, even those efforts were not entirely successful - CW still can't handle it. Then there was a final round getting it past two phase lookup. So as much as I would like to see this last header dependency eliminated, I think it would take a lot more time than it would first appear. And right now I don't have the time required even to address higher priority issues. Robert Ramey David Abrahams wrote:
"Robert Ramey" <ramey@rrsd.com> writes:
This is explained in the docmentation under
Reference/Special Considerations/Exporting Class Serialization
Robert Ramey
I have analyzed your issue and I believe there is a better way; one that avoids the header ordering requirement. I have put together a small demonstration, which is enclosed. This shows that we can instantiate a class template (instantiator) for the exported class and each archive that's been seen.
Then the only challenge is causing that class template instantiation to actually instantiate executable code. Fortunately, I've recently addressed that problem in changes to the serialization library. The basics are in CVS at:
boost/boost/concept/detail/general.hpp boost/boost/concept/detail/msvc.hpp boost/boost/concept/detail/borland.hpp
(Ignore the part that says BOOST_OLD_CONCEPT_SUPPORT), so we have the technology. And if you use the function pointer technique of BOOST_CONCEPT_ASSERT(( ... )) in boost/boost/concept/assert.hpp you'll be able to use BOOST_CLASS_EXPORT on class template instances with multiple arguments.
BOOST_CLASS_EXPORT(foo<bar,baz>) // an error, today.
I hope all this is helpful.
-- Dave Abrahams Boost Consulting www.boost-consulting.com
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
"Robert Ramey" <ramey@rrsd.com> writes:
I've given a quick look at your post and attachtment.
I vividly recall the making several attempts to get BOOST...EXPORT to compile - then to instantiate code. In fact, even those efforts were not entirely successful - CW still can't handle it. Then there was a final round getting it past two phase lookup.
Two phase lookup is a non-issue with the technique I posted, because everything is invoked with qualification and looked up from the point of the export, and it works beautifully with CW (both version 9.4 and 8.3), not to mention Comeau, Intel, GCC all the way back to 2.95, msvc back to 6, and Borland.
So as much as I would like to see this last header dependency eliminated, I think it would take a lot more time than it would first appear.
Well, this sounds really familiar to me for some reason. The fact remains that I've proven the workability of every single element of this solution. They are all well-understood (at least by me).
And right now I don't have the time required even to address higher priority issues.
Well, that's another matter. We're all out of time, I suppose. However, since I happen to be intimately familiar with what's required here, I'm willing to work up a complete patch for you if you'll tell me I won't be wasting my time. It would give me the chance to the "instantiate arbitrary code from a class template body" abstraction into a separate sub-library. ---- And if you don't like that idea, please allow me to offer this simplification of your export_generator: namespace export_impl { template <class T> struct instantiate_archive_for { template <class A> void operator()(A*) const { archive<A,T>::instantiate(); } }; } // namespace export_impl template<class T, class ASeq> struct export_generator { export_generator(){ boost::mpl::for_each<ASeq, add_pointer<_> >( export_impl::instantiate_archive_for<T>); } static const export_generator instance; }; The above solves no real problems for users other than slightly improved compile times, so you really should accept my first offer ;-) -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
"Robert Ramey" <ramey@rrsd.com> writes:
However, since I happen to be intimately familiar with what's required here, I'm willing to work up a complete patch for you if you'll tell me I won't be wasting my time. It would give me the chance to the "instantiate arbitrary code from a class template body" abstraction into a separate sub-library.
That would be ideal from my point of view - please feel free to check it in. Robert Ramey
"Robert Ramey" <ramey@rrsd.com> writes:
David Abrahams wrote:
"Robert Ramey" <ramey@rrsd.com> writes:
However, since I happen to be intimately familiar with what's required here, I'm willing to work up a complete patch for you if you'll tell me I won't be wasting my time. It would give me the chance to the "instantiate arbitrary code from a class template body" abstraction into a separate sub-library.
That would be ideal from my point of view - please feel free to check it in.
Great! I ask only that you update the documentation, to note that archive authors should invoke BOOST_SERIALIZATION_DECLARE_ARCHIVE to make their archives availabe to this mechanism, and to remove the warning about header ordering. You have two choices about how I'll handle backward compatibility: 1. Require that all user-written archive headers be updated with the next release of this library 2. Leave enough of the old mechanism in place so that existing code will still work as long as it follows the header ordering rule with respect to export.hpp okay, maybe three choices: 3. Tell me that archive authors outside the library *already* have to invoke some macro that I don't know about, and I can add the effect of BOOST_SERIALIZATION_DECLARE_ARCHIVE to that macro's implementation. Please let me know which of those angles I should pursue. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
"Robert Ramey" <ramey@rrsd.com> writes:
David Abrahams wrote:
"Robert Ramey" <ramey@rrsd.com> writes:
However, since I happen to be intimately familiar with what's required here, I'm willing to work up a complete patch for you if you'll tell me I won't be wasting my time. It would give me the chance to the "instantiate arbitrary code from a class template body" abstraction into a separate sub-library.
That would be ideal from my point of view - please feel free to check it in.
Great!
I ask only that you update the documentation, to note that archive authors should invoke BOOST_SERIALIZATION_DECLARE_ARCHIVE to make their archives availabe to this mechanism, and to remove the warning about header ordering.
You have two choices about how I'll handle backward compatibility:
1. Require that all user-written archive headers be updated with the next release of this library
2. Leave enough of the old mechanism in place so that existing code will still work as long as it follows the header ordering rule with respect to export.hpp
Either of the above would be just fine as far as I'm concerned. Go for whichever is easiest. From the mail on the user's list I don't think that many users actually create archives or derivations of existing ones. And adding a macro to an archive description is very small burden which occurs once per archive class - almost never. A small price to pay to eliminate the header order questions that the come up all the time. It would be nice if we could automatically detect that such a macro is absent and trap right there - but off hand I don't see a way to do this. We'll just have to add something to the documentation. Not a big deal.
okay, maybe three choices:
3. Tell me that archive authors outside the library *already* have to invoke some macro that I don't know about, and I can add the effect of BOOST_SERIALIZATION_DECLARE_ARCHIVE to that macro's implementation.
I don't think there is currently any such requirement. I would expect that most if not all archives derived from basic_oarchive.hpp or whatever. Maybe that might be useful - but as I said I don't see anyway to make the upgrade truely idiot-proof.
Please let me know which of those angles I should pursue.
So as I see it its the following a) replace export.hpp with a newer version. b) add a macro to each archive class declaration file c) re-work the test to cover the new header order d) update documentation to reflect the above. (I can do this) Robert Ramey
"Robert Ramey" <ramey@rrsd.com> writes:
You have two choices about how I'll handle backward compatibility:
1. Require that all user-written archive headers be updated with the next release of this library
2. Leave enough of the old mechanism in place so that existing code will still work as long as it follows the header ordering rule with respect to export.hpp
Either of the above would be just fine as far as I'm concerned. Go for whichever is easiest.
1 is easiest.
From the mail on the user's list I don't think that many users actually create archives or derivations of existing ones. And adding a macro to an archive description is very small burden which occurs once per archive class - almost never. A small price to pay to eliminate the header order questions that the come up all the time. It would be nice if we could automatically detect that such a macro is absent and trap right there
I could probably do that at archive construction time, but I'd rather not add that layer of complication.
- but off hand I don't see a way to do this. We'll just have to add something to the documentation. Not a big deal.
okay, maybe three choices:
3. Tell me that archive authors outside the library *already* have to invoke some macro that I don't know about, and I can add the effect of BOOST_SERIALIZATION_DECLARE_ARCHIVE to that macro's implementation.
I don't think there is currently any such requirement. I would expect that most if not all archives derived from basic_oarchive.hpp or whatever. Maybe that might be useful - but as I said I don't see anyway to make the upgrade truely idiot-proof.
Well option 2 is pretty good in that department.
Please let me know which of those angles I should pursue.
So as I see it its the following
a) replace export.hpp with a newer version. b) add a macro to each archive class declaration file c) re-work the test to cover the new header order d) update documentation to reflect the above. (I can do this)
Although I don't mind doing c), I would appreciate it if you'd rework the test yourself, before I start. This isn't out of laziness, but out of a desire to be sure I know I've met your requirements and expectations. If I rework the test, I could implement something that you'll find unacceptable. You could put the new test on a branch to avoid breaking the regressions. Thanks for your cooperation. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
"Robert Ramey" <ramey@rrsd.com> writes:
You have two choices about how I'll handle backward compatibility:
1. Require that all user-written archive headers be updated with the next release of this library
2. Leave enough of the old mechanism in place so that existing code will still work as long as it follows the header ordering rule with respect to export.hpp
Either of the above would be just fine as far as I'm concerned. Go for whichever is easiest.
1 is easiest.
Well option 2 is pretty good in that department.
I don't think its worth any extra effort to avoid breaking existent archives in this trivial to fix anyway. And I would hope the the newer export.hpp is simpler than the current one is which is way overly complex. I just had to keep making it more elaborate as each compiler was added in. Factoring out some of this complexity into your "? library" would be a significant benefit from where I stand.
c) re-work the test to cover the new header order
Although I don't mind doing c), I would appreciate it if you'd rework the test yourself, before I start.
Hmm - I was just thinking the the header order in test_export.cpp would be inverted. This would break test_export until the new export.hpp was in. Again not big deal as far as I'm concerned since I don't think tests are even being run on the HEAD right now. If it turns out that more needs to be done to make the test past, then that would be sort of a red flag indicating that users would have to change all thier programs (not just archive_class headers) which shouldn't be necessary if I understand your proposal correctly. Note that I'm also asuming that no new information is added to the archive itself - which would create quite a splash. For this reason, (aside from the fact that serialization has more or less exhausted its budget for testing resources). I think this is better than making a new test. It will verify the user code won't have to be changed. Robert Ramey
"Robert Ramey" <ramey@rrsd.com> writes:
Although I don't mind doing c), I would appreciate it if you'd rework the test yourself, before I start.
Hmm - I was just thinking the the header order in test_export.cpp would be inverted.
That's what I thought, too, but I wanted to make absolutely sure I didn't have a different interpretation than you. This should be trivial for you to accomplish.
This would break test_export until the new export.hpp was in.
That's why I wrote: You could put the new test on a branch to avoid breaking the regressions. Did you miss that? In case you need help: cvs tag -b serialization_header_order test_export.cpp cvs up -r serialization_header_order test_export.cpp cvs edit test_export.cpp <edit the test> cvs commit -m "test for dave" test_export.cpp cvs up -A test_export.cpp Now you're back to the HEAD.
Again not big deal as far as I'm concerned since I don't think tests are even being run on the HEAD right now. If it turns out that more needs to be done to make the test past, then that would be sort of a red flag indicating that users would have to change all thier programs (not just archive_class headers) which shouldn't be necessary if I understand your proposal correctly.
Exactly. That's why I want you to make exactly the acceptable change to the test.
Note that I'm also asuming that no new information is added to the archive itself - which would create quite a splash.
For this reason, (aside from the fact that serialization has more or less exhausted its budget for testing resources). I think this is better than making a new test. It will verify the user code won't have to be changed.
I never meant to suggest making a new test. I was suggesting that you change the test yourself. -- Dave Abrahams Boost Consulting www.boost-consulting.com
done Robert Ramey David Abrahams wrote:
"Robert Ramey" <ramey@rrsd.com> writes:
Although I don't mind doing c), I would appreciate it if you'd rework the test yourself, before I start.
Hmm - I was just thinking the the header order in test_export.cpp would be inverted.
That's what I thought, too, but I wanted to make absolutely sure I didn't have a different interpretation than you. This should be trivial for you to accomplish.
This would break test_export until the new export.hpp was in.
That's why I wrote:
You could put the new test on a branch to avoid breaking the regressions.
Did you miss that?
In case you need help:
cvs tag -b serialization_header_order test_export.cpp cvs up -r serialization_header_order test_export.cpp cvs edit test_export.cpp
<edit the test>
cvs commit -m "test for dave" test_export.cpp cvs up -A test_export.cpp
Now you're back to the HEAD.
Again not big deal as far as I'm concerned since I don't think tests are even being run on the HEAD right now. If it turns out that more needs to be done to make the test past, then that would be sort of a red flag indicating that users would have to change all thier programs (not just archive_class headers) which shouldn't be necessary if I understand your proposal correctly.
Exactly. That's why I want you to make exactly the acceptable change to the test.
Note that I'm also asuming that no new information is added to the archive itself - which would create quite a splash.
For this reason, (aside from the fact that serialization has more or less exhausted its budget for testing resources). I think this is better than making a new test. It will verify the user code won't have to be changed.
I never meant to suggest making a new test. I was suggesting that you change the test yourself.
At 7:47 AM -0700 5/3/06, Robert Ramey wrote:
David Abrahams wrote:
You have two choices about how I'll handle backward compatibility:
1. Require that all user-written archive headers be updated with the next release of this library
2. Leave enough of the old mechanism in place so that existing code will still work as long as it follows the header ordering rule with respect to export.hpp
Either of the above would be just fine as far as I'm concerned. Go for whichever is easiest. From the mail on the user's list I don't think that many users actually create archives or derivations of existing ones. And adding a macro to an archive description is very small burden which occurs once per archive class - almost never. A small price to pay to eliminate the header order questions that the come up all the time.
Speaking as one of those rare users who is creating or deriving from existing archives (the latter presently), we're comfortable with option 1, even though it is not backward compatible.
participants (4)
-
David Abrahams -
Kim Barrett -
Robert Ramey -
Thorsten Ottosen