AMDG *I vote to accept XInt into Boost* Overall: * The basic interface of the library is solid. * All thread-safety issues *must* be addressed. General: It should be possible to #include any header by itself. I don't like the fact that you split up the library into separate headers all of which can only be included from the main header. I don't have a problem with providing an option to enable COW, however: * It should not be used by default * If it is off, it should be off completely. Currently, the threadsafe option doesn't seem to have any effect on COW. boost::xint::integer is totally not thread-safe, because of the race on the reference count. Not only that, constructing an integer references a global variable in magnitude_manager_t::zerodata. This means that the library currently cannot safely be used with multiple threads at all, regardless of whether I try to prevent sharing or not. Thoughts on expression templates: Expression templates are a powerfull tool. However, I'm not convinced that they're appropriate for this library. Although they allow some clever optimizations, the extra complexity can cause confusion. This is a fairly basic library which we can expect to be used by people who don't know the language well enough to cope with the problems easily. Thoughts on separating out the arithmetic algorithms: * From an interface standpoint, arithmetic algorithms that can operate on arbitrary ranges are orthogonal to the functionality that the library provides. An big integer type that is a (mostly) drop in replacement for int is important by itself. IMHO, we should be reviewing the library that was submitted, not some hypothetical library that has not been written and has very different goals. * I think this is a much harder problem than those demanding this feature seem to think. Just off the top of my head: - Memory management can't just be ignored. Unlike the STL algorithms, the amount of space required for the results is dependent on the values involved. It isn't a direct function of the size of the input. Not to mention that all but the most basic functions need to create intermediate results. - Different representations of integers can't easily be encapsulated behind a concept that the generic algorithms can use. There are a quite a few possible representations. A class can pick one representation and stick with it. Generic algorithms would need to handle anything. I'd like to be able to make a fixedsize integer that uses only stack storage. There are two reasons for this: * Efficiency. This has already been mentioned by others. This is not a high priority for me, since it can always be optimized without affecting the interface. * Exception safety. I have some cases where I need intermediate results larger than any built-in integer type. However, I can easily find an upper bound on values I need to work with (Usually twice the size of whatever type I was given). I need to guarantee that these operations cannot fail. Just using nothrow is not an option, since NaN is as bad as an exception here. I think I agree that is_prime should be called is_probably_prime. exceptions.hpp: What is the rationale for on_exception? normally you should just use BOOST_THROW_EXCEPTION. integer.hpp Virtual inheritance is probably wrong. It's only needed with a diamond inheritence graph. Using chained inheritance instead of multiple inheritance probably works better with the empty base optimization. What is the correct way to specify the allocator? The example in the documentation shows my_allocator<boost::xint::detail::digit_t>, but I don't think that I as a user should ever need to refer to namespace detail. Can I use a Boost.Interprocess allocator to put an integer_t in shared memory? I doubt it since I don't see any constructors taking an allocator. line 268: if (str[0] == tnan[0] && std::basic_string<charT>(str) == tnan) You can compare a std::string directly to a const char*. Why can the comparison operators throw? I would have expected that they could be implemented to never throw. The comparison and arithmetic operators should be restricted with SFINAE. As written, they will allow comparison with a string. Is that intended? When you test for integer_t<BOOST_XINT_APARAMS>::Nothrow && n.is_nan(), you could just test n.is_nan() and make sure that n.is_nan() is always false if Nothrow is true. is_nan should be a free function, so it can overload with is_nan for other types. random.hpp: Watch out for ADL issues. Making strong_random_generator a typedef for a type in namespace detail means that boost::xint::detail is an associated namespace and boost::xint in *not*. I don't think that base_random_generator is needed anymore. addsubtract.hpp basic_types_and_includes.hpp digit_t: "It must contain at least half the number of bits that a doubledigit_t can hold," const std::size_t bits_per_digit = std::numeric_limits<digit_t>::digits; These two statements are somewhat contradictory. I believe that you have to guarantee that the result of multiplying any two digits together fits in a doubledigit_t. If digit_t holds more than half the number of bits of a doubledigit_t, then this won't hold. Of course this is rather hypothetical, since we always work with powers of 2. magnitude_t: * It probably needs to be POD for you to make assumptions about the layout. * Can't you make the assignment operator private instead of having it throw? Actually if this is to avoid C4512, you're better off disabling the warning. convert.hpp: You assume that the letters are contiguous. While this is usually true, it is not guaranteed. The only guarantee that the standard gives about the character set is that the digits 0-9 are contiguous. to_string uses the global locale to convert char to charT. from_string uses implicit conversion. Be consistent. gcd.hpp: The behavior of invmod when the inverse doesn't exist is to return zero. Wouldn't an exception/NaN be more consistant? internals.hpp: You should probably check BOOST_NO_EXCEPTIONS (from boost/config.hpp) in addition to BOOST_XINT_NO_EXCEPTIONS. log2.hpp: What about using boost/pending/integer_log2.hpp? Perhaps this should also be called integer_log2, to avoid confusion with std::log2 which returns a floating point value. prime.hpp is_prime should take a generator instead of creating its own. That way random_prime can use the same generator for everything. randomgen.hpp: Do you want the /dev/urandom implementation to be buffered? Boost.Random intentionally uses raw file descriptors to avoid reading more random bytes than are actually needed. streams.hpp: Even though you templated the stream operators on the traits, they won't compile with anything other than std::char_traits, because to_string uses the default traits, and when you send a string to a stream, the traits have to match. I don't think std::showbase works correctly with std::setw. (This is a very common problem with ostream operators) Make sure that you handle std::internal, too. operator>> should skip leading whitespace (depending on whether this is set for the stream of course.) Tests: You don't need to create an alias to run the tests. Just use run test_streams.cpp ; run test_add.cpp ; ... I've written a few extra test cases. The ones called fail_*.cpp should be marked as compile-fail, the ones called test should pass. Beyond that, here are a few more things that I thought were missing from the tests: All the comparison operators. Not just operator< and operator>. Check the behavior of <, >, <=, and >= for equal arguments. Check cases where the operators should return false. In Christ, Steven Watanabe
On Mon, 7 Mar 2011 17:15:58 -0800 Steven Watanabe <watanabesj@gmail.com> wrote:
*I vote to accept XInt into Boost*
Thank you for the review, and your extensive comments.
General: It should be possible to #include any header by itself. I don't like the fact that you split up the library into separate headers all of which can only be included from the main header.
Yes, a lot of the functions can be split off if the client code won't need them. I've made a note to do so soon. There's already an include-everything header, so that won't break client code even if I can't get it in the first Boost version.
I don't have a problem with providing an option to enable COW, however: * It should not be used by default * If it is off, it should be off completely. Currently, the threadsafe option doesn't seem to have any effect on COW.
Only to disable it on objects returned from the library. Which is all that should be needed for thread safety, more on this below.
boost::xint::integer is totally not thread-safe, because of the race on the reference count.
I recently swore to eat my keyboard without salt if anyone found a flaw in the thread-safety design (rather than an implementation bug). I like this keyboard, but I doubt it would taste very good, especially without any salt. Every integer_t object will have unique storage when it's returned to user code (assuming the threadsafe option isn't disabled), and no two threads will ever have access the same integer_t object within the library. With that in mind, should I get the knife and fork ready?
Not only that, constructing an integer references a global variable in magnitude_manager_t::zerodata. This means that the library currently cannot safely be used with multiple threads at all, regardless of whether I try to prevent sharing or not.
That's intended as read-only static data. The inc/dec functions were modifying the copy_count though, so you're right. I've changed it so that the copy_count member is never altered on objects marked read-only either. If CoW survives, I'll find a better solution.
Thoughts on separating out the arithmetic algorithms: [...] * I think this is a much harder problem than those demanding this feature seem to think.
Just off the top of my head: - Memory management can't just be ignored. Unlike the STL algorithms, the amount of space required for the results is dependent on the values involved. It isn't a direct function of the size of the input. Not to mention that all but the most basic functions need to create intermediate results.
The design I see would require all such user-supplied magnitude types to have a reallocation function that the library can call. If that function doesn't provide the memory requested, the user gets only the lower bits of the correct result. The code for handling that is already in the library, for the fixed-length integers.
- Different representations of integers can't easily be encapsulated behind a concept that the generic algorithms can use. There are a quite a few possible representations. A class can pick one representation and stick with it. Generic algorithms would need to handle anything.
I think that can be handled by putting the onus of dealing with it onto the person providing the new type.
I think I agree that is_prime should be called is_probably_prime.
Already done in my working copy.
exceptions.hpp:
What is the rationale for on_exception?
<http://www.oakcircle.com/xint_docs/exception_handler.html>
normally you should just use BOOST_THROW_EXCEPTION.
Hm... it looks like that could be used with -fno-exceptions. Added to the list.
integer.hpp
Virtual inheritance is probably wrong. It's only needed with a diamond inheritence graph.
No probably to it, CRTP is a much better choice. I'll be changing it to that for the next release.
What is the correct way to specify the allocator? The example in the documentation shows my_allocator<boost::xint::detail::digit_t>, but I don't think that I as a user should ever need to refer to namespace detail.
digit_t is already slated to be moved to the boost::xint namespace in the next release.
Can I use a Boost.Interprocess allocator to put an integer_t in shared memory? I doubt it since I don't see any constructors taking an allocator.
I've never tried it, but if it has the standard allocator interface, I believe you can. Just specify the allocator class as a parameter to the integer_t class template. (The documentation for that has already been improved for the next release.)
Why can the comparison operators throw? I would have expected that they could be implemented to never throw.
So far as I know, the only reason they would ever throw is if they're passed a Not-a-Number value, or if something unpredictable has gone seriously wrong.
The comparison and arithmetic operators should be restricted with SFINAE. As written, they will allow comparison with a string. Is that intended?
It would be highly unusual, but not specifically an error, assuming the string contains a base-ten numeric value.
When you test for integer_t<BOOST_XINT_APARAMS>::Nothrow && n.is_nan(), you could just test n.is_nan() and make sure that n.is_nan() is always false if Nothrow is true.
Couldn't that prevent some less-intelligent compilers from optimizing out the test?
basic_types_and_includes.hpp
digit_t: "It must contain at least half the number of bits that a doubledigit_t can hold," const std::size_t bits_per_digit = std::numeric_limits<digit_t>::digits;
These two statements are somewhat contradictory. I believe that you have to guarantee that the result of multiplying any two digits together fits in a doubledigit_t. If digit_t holds more than half the number of bits of a doubledigit_t, then this won't hold.
You're right, the comment is mistaken.
Of course this is rather hypothetical, since we always work with powers of 2.
Those types, and the originally-arcane methods used to come up with them, were intended to support *any* platform, no matter how odd. I've heard horror stories of systems with seventeen-bit words and nine-bit characters. :-)
magnitude_t: * It probably needs to be POD for you to make assumptions about the layout.
The "struct hack" is a standard C idiom, and has been for as long as C has been around. It only became explicitly documented behavior in C99, but it works with all known C and C++ compilers; I checked very thoroughly, and even asked on this list (where one might reasonably expect to find people with experience with really odd architectures and compiler setups), and wasn't able to find a single report of a compiler where it would fail.
* Can't you make the assignment operator private instead of having it throw? Actually if this is to avoid C4512, you're better off disabling the warning.
I wanted something that would fail loudly if the library attempted to use it. Making it private would probably work even better.
gcd.hpp: The behavior of invmod when the inverse doesn't exist is to return zero. Wouldn't an exception/NaN be more consistant?
I prefer to reserve exceptions for exceptional behavior, primarily errors. A number that doesn't have an inverse isn't really exceptional, there are many such values.
log2.hpp: What about using boost/pending/integer_log2.hpp? Perhaps this should also be called integer_log2, to avoid confusion with std::log2 which returns a floating point value.
The ones in that file are for internal use only, but your point is valid for the log2 function in integer.cpp.
prime.hpp
is_prime should take a generator instead of creating its own. That way random_prime can use the same generator for everything.
I'd forgotten that was even there. :-( You're right, it should... and now does, optionally.
I don't think std::showbase works correctly with std::setw. (This is a very common problem with ostream operators) [...]
It looks like the standard library mostly works the way my code does, but has some quirks with zero values that I'm not presently emulating, according to <http://stackoverflow.com/questions/3273654/ostream-showbase-non-present-for-zero-value-and-internal-doesnt-work-for-handle>.
I've written a few extra test cases. The ones called fail_*.cpp should be marked as compile-fail, the ones called test should pass.
GCC balks at some of those tests here (haven't tried MSVC yet). Maybe you can see why? In test_invalid_overload.cpp: Line 34 says "result.message() =", which the compiler won't accept. I'm not sure what is intended there. The compiler balked at lines 54 and onward ("error: no matching function for call to ‘boost::xint::integer_t<>::integer_t(const unrelated_type&)’"). No idea how to fix it, other than removing unrelated_type from the mpl::vector. The test_complement case reports "error: cannot pass objects of non-trivially-copyable type ‘class boost::xint::integer’ through ‘...’". I was able to get test_arithmetic_conversion.cpp to compile and work, but only after changing the uniform_int_distribution.hpp include to uniform_int.hpp instead, and tweaking the first two lines of the test_arithmetic function. I'm not using the very latest Boost yet, but I'm using a very recent one (1.45), so I'm not sure what happened there. Does fail_adl_detail.cpp compile on your system? It shouldn't, so far as I can see, but on mine it does. Again, thank you. -- Chad Nelson Oak Circle Software, Inc. * * *
Chad Nelson wrote:
Steven Watanabe <watanabesj@gmail.com> wrote:
Why can the comparison operators throw? I would have expected that they could be implemented to never throw.
So far as I know, the only reason they would ever throw is if they're passed a Not-a-Number value, or if something unpredictable has gone seriously wrong.
I would hope you could be more definitive than that and clearly document the behavior.
When you test for integer_t<BOOST_XINT_APARAMS>::Nothrow && n.is_nan(), you could just test n.is_nan() and make sure that n.is_nan() is always false if Nothrow is true.
Couldn't that prevent some less-intelligent compilers from optimizing out the test?
Only if is_nan() isn't inline or the optimizer is really stupid (in which case you shouldn't care because there will be many other failures to optimize).
magnitude_t: * It probably needs to be POD for you to make assumptions about the layout.
The "struct hack" is a standard C idiom, and has been for as long as C has been around. It only became explicitly documented behavior in C99, but it works with all known C and C++ compilers; I checked very thoroughly, and even asked on this list (where one might reasonably expect to find people with experience with really odd architectures and compiler setups), and wasn't able to find a single report of a compiler where it would fail.
A C struct is POD.
gcd.hpp: The behavior of invmod when the inverse doesn't exist is to return zero. Wouldn't an exception/NaN be more consistant?
I prefer to reserve exceptions for exceptional behavior, primarily errors. A number that doesn't have an inverse isn't really exceptional, there are many such values.
Returning NaN poisons subsequent calculations. Zero doesn't. _____ Rob Stewart Software Engineer, Core Software Susquehanna International Group, LLP IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
On Wed, 9 Mar 2011 09:41:58 -0500 "Stewart, Robert" <Robert.Stewart@sig.com> wrote:
Why can the comparison operators throw? I would have expected that they could be implemented to never throw.
So far as I know, the only reason they would ever throw is if they're passed a Not-a-Number value, or if something unpredictable has gone seriously wrong.
I would hope you could be more definitive than that and clearly document the behavior.
I can be definitive about the Not-a-Number behavior, and anything else that I put into the code. I can't be definitive about anything unpredictable. Out-of-memory isn't a problem in the comparison functions, but it's probably the most common example of stuff that can go wrong unpredictably.
magnitude_t: * It probably needs to be POD for you to make assumptions about the layout.
The "struct hack" is a standard C idiom [...]
A C struct is POD.
So far as I've been able to determine, a C++ struct with some non-virtual functions, and that doesn't inherit from anything, acts like POD in every case. There's no need for a vtable or any other appended data that I've been able to imagine, and there certainly isn't on the two compilers I've directly tested (MSVC on Windows and GCC on Ubuntu Linux). -- Chad Nelson Oak Circle Software, Inc. * * *
On Wed, Mar 9, 2011 at 07:52, Chad Nelson <chad.thecomfychair@gmail.com> wrote:
On Wed, 9 Mar 2011 09:41:58 -0500 "Stewart, Robert" <Robert.Stewart@sig.com> wrote:
magnitude_t: * It probably needs to be POD for you to make assumptions about the layout.
The "struct hack" is a standard C idiom [...]
A C struct is POD.
So far as I've been able to determine, a C++ struct with some non-virtual functions, and that doesn't inherit from anything, acts like POD in every case. There's no need for a vtable or any other appended data that I've been able to imagine, and there certainly isn't on the two compilers I've directly tested (MSVC on Windows and GCC on Ubuntu Linux).
C++0x offers the definition of a "standard-layout class" in 9 [class]/7, with layout specified by 9.2 [class.mem]. Precise requirements: — has no non-static data members of type non-standard-layout class (or array of such types) or reference, — has no virtual functions (10.3) and no virtual base classes (10.1), — has the same access control (Clause 11) for all non-static data members, — has no non-standard-layout base classes, — either has no non-static data members in the most derived class and at most one base class with non-static data members, or has no base classes with non-static data members, and — has no base classes of the same type as the first non-static data member.
At Wed, 9 Mar 2011 10:52:35 -0500, Chad Nelson wrote:
So far as I've been able to determine, a C++ struct with some non-virtual functions, and that doesn't inherit from anything, acts like POD in every case. There's no need for a vtable or any other appended data that I've been able to imagine, and there certainly isn't on the two compilers I've directly tested (MSVC on Windows and GCC on Ubuntu Linux).
Hi Chad, That's really not a good enough reason to depend on it. Except in very exceptional circumstances, we try to ensure that all code in Boost conforms to the letter of the standard, rather than relying on the actual behavior of popular implementations. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
AMDG On 03/09/2011 06:13 AM, Chad Nelson wrote:
boost::xint::integer is totally not thread-safe, because of the race on the reference count. I recently swore to eat my keyboard without salt if anyone found a flaw in the thread-safety design (rather than an implementation bug). I like
On Mon, 7 Mar 2011 17:15:58 -0800 Steven Watanabe<watanabesj@gmail.com> wrote: this keyboard, but I doubt it would taste very good, especially without any salt.
Every integer_t object will have unique storage when it's returned to user code (assuming the threadsafe option isn't disabled),
... by anything except the copy constructor. The copy constructor does *not* call make_unique.
and no two threads will ever have access the same integer_t object within the library. With that in mind, should I get the knife and fork ready?
Stepping through this code in the debugger shows copy_count=2 at the end of main: #include <boost/xint/xint.hpp> int main() { typedef boost::xint::integer_t<boost::xint::options::threadsafe> test_t; test_t test1(1); test_t test2(test1); }
Can I use a Boost.Interprocess allocator to put an integer_t in shared memory? I doubt it since I don't see any constructors taking an allocator. I've never tried it, but if it has the standard allocator interface, I believe you can. Just specify the allocator class as a parameter to the integer_t class template. (The documentation for that has already been improved for the next release.)
Interprocess has a few extra requirements on allocators: * The allocators are stateful. You cannot assume that allocators of the same type are equivalent. * The containers must store the Allocator::pointer type, not T*.
Why can the comparison operators throw? I would have expected that they could be implemented to never throw. So far as I know, the only reason they would ever throw is if they're passed a Not-a-Number value, or if something unpredictable has gone seriously wrong.
If the integer is nothrow, then comparison doesn't throw. Otherwise, the argument cannot be NaN. There should be nothing unpredictable, since you don't call user-defined code, nor is there any memory allocation. Exceptions don't randomly come out of nowhere.
The comparison and arithmetic operators should be restricted with SFINAE. As written, they will allow comparison with a string. Is that intended? It would be highly unusual, but not specifically an error, assuming the string contains a base-ten numeric value.
I just think that the extra operators that you allow should match the implicit conversions. Since the constructor that takes a string is explicit, I should have to cast the string to an integer.
When you test for integer_t<BOOST_XINT_APARAMS>::Nothrow&& n.is_nan(), you could just test n.is_nan() and make sure that n.is_nan() is always false if Nothrow is true. Couldn't that prevent some less-intelligent compilers from optimizing out the test?
I wouldn't worry about it. This is a fairly easy optimization.
Of course this is rather hypothetical, since we always work with powers of 2. Those types, and the originally-arcane methods used to come up with them, were intended to support *any* platform, no matter how odd. I've heard horror stories of systems with seventeen-bit words and nine-bit characters. :-)
I think it won't quite work, and I have little confidence that it will ever work unless you actually test on such a system. It's too easy to make hidden assumptions.
magnitude_t: * It probably needs to be POD for you to make assumptions about the layout. The "struct hack" is a standard C idiom, and has been for as long as C has been around. It only became explicitly documented behavior in C99, but it works with all known C and C++ compilers; I checked very thoroughly, and even asked on this list (where one might reasonably expect to find people with experience with really odd architectures and compiler setups), and wasn't able to find a single report of a compiler where it would fail.
I don't object to the 'struct hack.' I object to using it for something that isn't a struct.
gcd.hpp: The behavior of invmod when the inverse doesn't exist is to return zero. Wouldn't an exception/NaN be more consistant? I prefer to reserve exceptions for exceptional behavior, primarily errors. A number that doesn't have an inverse isn't really exceptional, there are many such values.
I think that returning 0 has the potential to be more surprising. From the name I would have assumed that the postcondition of invmod is that it returns the inverse. If it cannot meet this, then it should throw. This is safer for anyone who tries to use it without thinking about whether the inverse exists. If I call invmod when there is no inverse, it is probably wrong to go on my merry way as though the inverse were 0.
I don't think std::showbase works correctly with std::setw. (This is a very common problem with ostream operators) [...] It looks like the standard library mostly works the way my code does, but has some quirks with zero values that I'm not presently emulating, according to <http://stackoverflow.com/questions/3273654/ostream-showbase-non-present-for-zero-value-and-internal-doesnt-work-for-handle>.
The issue is that if you split up the streaming into multiple parts, std::setw will be applied to the first string that you print, rather than the whole, which kind of messes up the formatting.
I've written a few extra test cases. The ones called fail_*.cpp should be marked as compile-fail, the ones called test should pass. GCC balks at some of those tests here (haven't tried MSVC yet). Maybe you can see why? In test_invalid_overload.cpp:
Line 34 says "result.message() =", which the compiler won't accept. I'm not sure what is intended there.
The compiler balked at lines 54 and onward ("error: no matching function for call to ‘boost::xint::integer_t<>::integer_t(const unrelated_type&)’"). No idea how to fix it, other than removing unrelated_type from the mpl::vector.
The test_complement case reports "error: cannot pass objects of non-trivially-copyable type ‘class boost::xint::integer’ through ‘...’".
I only tested with MSVC 10. Looks like gcc is pickier about it. Try changing the constructor of any to be a template instead of using '...'.
I was able to get test_arithmetic_conversion.cpp to compile and work, but only after changing the uniform_int_distribution.hpp include to uniform_int.hpp instead, and tweaking the first two lines of the test_arithmetic function. I'm not using the very latest Boost yet, but I'm using a very recent one (1.45), so I'm not sure what happened there.
I'm using the trunk which has some major updates to Boost.Random. uniform_int is fine, but you'll need to specify the bounds explicitly.
Does fail_adl_detail.cpp compile on your system? It shouldn't, so far as I can see, but on mine it does.
It compiles for me too. This is one of my favorite tests which almost always fails unless you've thought about it. In Christ, Steven Watanabe
On Wed, 09 Mar 2011 10:55:40 -0800 Steven Watanabe <watanabesj@gmail.com> wrote:
boost::xint::integer is totally not thread-safe, because of the race on the reference count.
I recently swore to eat my keyboard without salt if anyone found a flaw in the thread-safety design (rather than an implementation bug). I like this keyboard, but I doubt it would taste very good, especially without any salt.
Every integer_t object will have unique storage when it's returned to user code (assuming the threadsafe option isn't disabled),
... by anything except the copy constructor. The copy constructor does *not* call make_unique.
Ahh... my keyboard is safe then, it's an implementation error. :-) I've already corrected it for the next release.
Can I use a Boost.Interprocess allocator to put an integer_t in shared memory? I doubt it since I don't see any constructors taking an allocator. I've never tried it, but if it has the standard allocator interface, I believe you can. Just specify the allocator class as a parameter to the integer_t class template. (The documentation for that has already been improved for the next release.)
Interprocess has a few extra requirements on allocators: * The allocators are stateful. You cannot assume that allocators of the same type are equivalent. * The containers must store the Allocator::pointer type, not T*.
Then I'm not sure. They should be usable with it, but without trying it I can't say for certain.
The comparison and arithmetic operators should be restricted with SFINAE. As written, they will allow comparison with a string. Is that intended? It would be highly unusual, but not specifically an error, assuming the string contains a base-ten numeric value.
I just think that the extra operators that you allow should match the implicit conversions. Since the constructor that takes a string is explicit, I should have to cast the string to an integer.
Fixed.
Of course this is rather hypothetical, since we always work with powers of 2. Those types, and the originally-arcane methods used to come up with them, were intended to support *any* platform, no matter how odd. I've heard horror stories of systems with seventeen-bit words and nine-bit characters. :-)
I think it won't quite work, and I have little confidence that it will ever work unless you actually test on such a system. It's too easy to make hidden assumptions.
If I ever find myself with programming access to such a system, I'll try it. They seem pretty scarce these days.
GCC balks at some of those tests here (haven't tried MSVC yet). Maybe you can see why? In test_invalid_overload.cpp:
Line 34 says "result.message() =", which the compiler won't accept. I'm not sure what is intended there.
The compiler balked at lines 54 and onward ("error: no matching function for call to ‘boost::xint::integer_t<>::integer_t(const unrelated_type&)’"). No idea how to fix it, other than removing unrelated_type from the mpl::vector.
The test_complement case reports "error: cannot pass objects of non-trivially-copyable type ‘class boost::xint::integer’ through ‘...’".
I only tested with MSVC 10. Looks like gcc is pickier about it. Try changing the constructor of any to be a template instead of using '...'.
That took care of that problem. Any comments on the others? The second one seems important... I had to comment all three types out of the mpl::vector to get it to even compile, once I fixed the overloaded functions to only work on things that are integers. As far as I can see, that makes those tests useless.
Does fail_adl_detail.cpp compile on your system? It shouldn't, so far as I can see, but on mine it does.
It compiles for me too. This is one of my favorite tests which almost always fails unless you've thought about it.
Hang on... you're telling the main function to use the detail namespace, where that function is. Wouldn't it be found directly because of that, ignoring ADL? After removing that using line (and modifying the options), it's still being found though. It looks like it's because integer_t is inheriting from things in the xint::detail namespace. As far as I can see, that can't be eliminated without pulling more classes into the xint namespace. Do you have a solution that wouldn't require that? -- Chad Nelson Oak Circle Software, Inc. * * *
AMDG On 03/09/2011 07:22 PM, Chad Nelson wrote:
On Wed, 09 Mar 2011 10:55:40 -0800 Steven Watanabe<watanabesj@gmail.com> wrote:
GCC balks at some of those tests here (haven't tried MSVC yet). Maybe you can see why? In test_invalid_overload.cpp:
Line 34 says "result.message() =", which the compiler won't accept. I'm not sure what is intended there.
The compiler balked at lines 54 and onward ("error: no matching function for call to ‘boost::xint::integer_t<>::integer_t(const unrelated_type&)’"). No idea how to fix it, other than removing unrelated_type from the mpl::vector.
The test_complement case reports "error: cannot pass objects of non-trivially-copyable type ‘class boost::xint::integer’ through ‘...’".
I only tested with MSVC 10. Looks like gcc is pickier about it. Try changing the constructor of any to be a template instead of using '...'. That took care of that problem. Any comments on the others? The second one seems important... I had to comment all three types out of the mpl::vector to get it to even compile, once I fixed the overloaded functions to only work on things that are integers. As far as I can see, that makes those tests useless.
Ah, I see. The test will only work if you disable the operators directly. I expected to see something like: template<typename N, BOOST_XINT_CLASS_APARAMS> typename boost::enable_if<boost::is_integral<N>, bool>::type operator<(const integral_t<BOOST_XINT_APARAMS>& lhs, N rhs); In general you need to be more careful about the behaviour of operators W.R.T. overload resolution than with most other functions, because operators tend to be have more overloads out of your control.
Does fail_adl_detail.cpp compile on your system? It shouldn't, so far as I can see, but on mine it does. It compiles for me too. This is one of my favorite tests which almost always fails unless you've thought about it. Hang on... you're telling the main function to use the detail namespace, where that function is. Wouldn't it be found directly because of that, ignoring ADL?
No. The using directive refers to the options namespace.
After removing that using line (and modifying the options), it's still being found though. It looks like it's because integer_t is inheriting from things in the xint::detail namespace. As far as I can see, that can't be eliminated without pulling more classes into the xint namespace. Do you have a solution that wouldn't require that?
That's the solution I usually use. I just make sure that they don't show up in the documentation. Another option is to use an ADL barrier namespace. namespace detail { class C; } becomes namespace detail { namespace adl_barrier_for_C { class C; } using adl_barrier_for_C::C; } The point is to make sure that your implementation functions can never be found by an unqualified call outside the library. This doesn't often cause problems, but when it does it's really a pain to debug. I prefer to be very strict about controlling ADL. (On that note, it's generally a good idea to use (f)(x) or xint::f(x) instead of f(x) unless you intend f to be a customization point). In Christ, Steven Watanabe
On Thu, 10 Mar 2011 13:08:32 -0800 Steven Watanabe <watanabesj@gmail.com> wrote:
That took care of that problem. Any comments on the others? The second one seems important... I had to comment all three types out of the mpl::vector to get it to even compile, once I fixed the overloaded functions to only work on things that are integers. As far as I can see, that makes those tests useless.
Ah, I see. The test will only work if you disable the operators directly. [...]
That would be simpler than the work-around I used too. I've noted it for future implementation.
Does fail_adl_detail.cpp compile on your system? It shouldn't, so far as I can see, but on mine it does. It compiles for me too. This is one of my favorite tests which almost always fails unless you've thought about it. Hang on... you're telling the main function to use the detail namespace, where that function is. Wouldn't it be found directly because of that, ignoring ADL?
No. The using directive refers to the options namespace.
Yes, I realized my mistake this morning. Sorry about that.
After removing that using line (and modifying the options), it's still being found though. It looks like it's because integer_t is inheriting from things in the xint::detail namespace. As far as I can see, that can't be eliminated without pulling more classes into the xint namespace. Do you have a solution that wouldn't require that?
That's the solution I usually use. I just make sure that they don't show up in the documentation. Another option is to use an ADL barrier namespace. [...]
Nice.
(On that note, it's generally a good idea to use (f)(x) or xint::f(x) instead of f(x) unless you intend f to be a customization point).
Sorry, you lost me. Would all function calls need that? If not, which ones is it limited to? -- Chad Nelson Oak Circle Software, Inc. * * *
AMDG On 03/10/2011 04:54 PM, Chad Nelson wrote:
On Thu, 10 Mar 2011 13:08:32 -0800 Steven Watanabe<watanabesj@gmail.com> wrote:
(On that note, it's generally a good idea to use (f)(x) or xint::f(x) instead of f(x) unless you intend f to be a customization point). Sorry, you lost me. Would all function calls need that?
Only calls to free functions.
If not, which ones is it limited to?
Basically, this disables argument dependent lookup, so it should be used whenever ADL would be triggered and you do not explicitly want ADL. Looking through you code again, I think I was a bit hasty in suggesting this. I didn't see any cases where it would actually be important, because your functions all take your own types. Even if you get an extra overload from a user's namespace, your overloads should always be a better match. I still think that it's better to block ADL whenever possible, but in practice, I don't do so consistently, and most people don't worry about it. In Christ, Steven Watanabe
[I'm beginning a new thread, as the original subject doesn't really reflect the content of my message below.] On 3/9/2011 6:13 AM, Chad Nelson wrote:
On Mon, 7 Mar 2011 17:15:58 -0800 Steven Watanabe<watanabesj@gmail.com> wrote: [...]
Thoughts on separating out the arithmetic algorithms: [...] * I think this is a much harder problem than those demanding this feature seem to think.
You're right, Steven, it's likely not an easy task; there hasn't been much discussion about it, so there's a lot of unknowns. I guess we'll see how hard it is once we start trying.
Just off the top of my head: - Memory management can't just be ignored. Unlike the STL algorithms, the amount of space required for the results is dependent on the values involved. It isn't a direct function of the size of the input. Not to mention that all but the most basic functions need to create intermediate results.
At least the the basic arithmetic operations, we can give a reasonable upper bound on the size of the result given the size of the inputs. You can estimate the size of a sum or difference to within a carry digit (assuming no cancellation), and likewise for multiplication and division. For more complicated operations, yes, the unknown size of the result would complicate the interface. We might need to group algorithms into a number of different categories; for example, the following four: the size of the result is (nearly) precisely determined by the size of the inputs; the size of the result is bounded by (a function of) the size of the inputs; the size of the result can be determined from the inputs prior to constructing the result, with no or little loss in efficiency; and the size of the result is undetermined until it has been fully constructed.
The design I see would require all such user-supplied magnitude types to have a reallocation function that the library can call. If that function doesn't provide the memory requested, the user gets only the lower bits of the correct result. The code for handling that is already in the library, for the fixed-length integers.
I'd like to hear more specifics about this idea...if you have more specifics at the moment, that is.
- Different representations of integers can't easily be encapsulated behind a concept that the generic algorithms can use. There are a quite a few possible representations. A class can pick one representation and stick with it. Generic algorithms would need to handle anything.
I'm envisioning a "generic integer" is not much more than a range of digits representing the base B expansion. Possibly with an optional sign, or with an implied sign based on a 2's complement representation. Is that too vague, or would that not cover some reasonable integer implementations?
I think that can be handled by putting the onus of dealing with it onto the person providing the new type.
E.g., by supplying a policy class of fundamental operations...? For example, it seems for a generic addition algorithm, one needs to be able to add two digits and generate a truncated sum + carry bit. I can see at least two ways to do this, depending on if your digits are base 2^b or base 2^(b/2), where b is the bits in an unsigned int (say). The latter is easy to get a sum + carry in "regular" C++; the former is more compact and would likely benefit from some assembly language specializations to detect carries in additions. Since there isn't a canonical "add with carry" algorithm, it could be supplied by a policy parameter. [Maybe there *is* a canonical "add with carry" algorithm, but hopefully the overall point is clear.] Perhaps such discussions on the design of a generic interface are best left for after the review. I couldn't help but throw some ideas out there already, though ;) - Jeff
On Wed, 09 Mar 2011 11:25:57 -0800 "Jeffrey Lee Hellrung, Jr." <jhellrung@ucla.edu> wrote:
The design I see would require all such user-supplied magnitude types to have a reallocation function that the library can call. If that function doesn't provide the memory requested, the user gets only the lower bits of the correct result. The code for handling that is already in the library, for the fixed-length integers.
I'd like to hear more specifics about this idea...if you have more specifics at the moment, that is.
Only the design in my head. XInt would require a few things of the user-specified type: it must have functions for setting and retrieving the sign and the used memory, accessing the memory, and getting the current reserved size of the memory. It must have a function for allocating more memory, which can be a no-op if you don't want the type to get any larger (though it won't guarantee the right answer in that case). It must have a typedef for the digits of the magnitude, and that type must have properties similar to xint::digit_t. Probably a few other things, which I won't know until I actually try writing code to use it.
I think that can be handled by putting the onus of dealing with it onto the person providing the new type.
E.g., by supplying a policy class of fundamental operations...? [...]
However the implementor desires, so long as it provides sign and magnitude in the way that the functions can use.
Perhaps such discussions on the design of a generic interface are best left for after the review. I couldn't help but throw some ideas out there already, though ;)
Yes, there's plenty to do before I even seriously think about that. -- Chad Nelson Oak Circle Software, Inc. * * *
AMDG On 03/09/2011 06:13 AM, Chad Nelson wrote:
On Mon, 7 Mar 2011 17:15:58 -0800 Steven Watanabe<watanabesj@gmail.com> wrote:
exceptions.hpp:
What is the rationale for on_exception? <http://www.oakcircle.com/xint_docs/exception_handler.html>
We already have BOOST_THROW_EXCEPTION. In Christ, Steven Watanabe
participants (6)
-
Chad Nelson -
Dave Abrahams -
Jeffrey Lee Hellrung, Jr. -
Scott McMurray -
Steven Watanabe -
Stewart, Robert