Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [cdt-dev] Bug ID 403404

Hi Thomas!

Thanks for your valuable feedback.

Yes indeed it is possible to compare two booleans!  Going back in my history when writing the code I see that I misinterpreted a syntax given by the Eclipse IDE. This left me somewhat puzzled at first but I see my misunderstanding now.

Going back to your first comment you are right regarding the fact that the benefit of the class could be achieved with a simple wrapper for along without the bit_63 boolean. This would provide better invariability. In fact you can return the product of the signed multiply without any checking and this would yield the good result for the unsigned long multiply. At least for the low part of the multiply result. So getting a unsigned long type with a simple wrapper class is not a big deal which was the purpose of the exercise for me.

I'll try to see if I can come up with something with a signed 128 bits class as an exercise. If you think some methods are missing in the UnsignedLong class for a full 128 bits signed implementation then let me know!


Regards
Guy

Inactive hide details for "Corbat Thomas (tcorbat@xxxxxx)" ---04/11/2013 02:53:55 AM---Hi Guy What I meant with the invariant, "Corbat Thomas (tcorbat@xxxxxx)" ---04/11/2013 02:53:55 AM---Hi Guy What I meant with the invariant, at least what I would expect: The data in the class should a



Hi Guy
 
What I meant with the invariant, at least what I would expect: The data in the class should always be consistent regarding the value in bit_63 and the most significant bit of ulongValue. I would expect, if the value of UnsignedLong exceeds Long.MAX_VALUE the most significant bit to be set to 1 and bit_63 to be set to true. Otherwise 0 and false respectively. While this is given for example when calling castulong(Long.MIN_VALUE) it is not when you use the constructor UnsignedLong(0, 1). But that might be just a matter of implementing the constructor differently.
 
Why is it not possible to compare two booleans with != operator?
 
I guess if we had an implementation which is distinguishable regarding the sign and is capable of handling at least 128bit integer we could cover most real cases. And if not it would probably be extendible easily for wider types. But I don't know whether that is far from using BitInteger eventually.
 
Regards
Thomas
 
 
From: cdt-dev-bounces@xxxxxxxxxxx [mailto:cdt-dev-bounces@xxxxxxxxxxx] On Behalf Of Guy.Bonneau@xxxxxxxxxxx
Sent: Mittwoch, 10. April 2013 18:43
To: CDT General developers list.
Subject: Re: [cdt-dev] Bug ID 403404
 

See my answers below

Inactive hide details for "Corbat Thomas (tcorbat@xxxxxx)" ---04/10/2013 03:27:11 AM---Hi Guy I just had a glance at the code o"Corbat Thomas (tcorbat@xxxxxx)" ---04/10/2013 03:27:11 AM---Hi Guy I just had a glance at the code of UnsignedLong. Just my observations:


Hi Guy

I just had a glance at the code of UnsignedLong. Just my observations:
- The first thing that popped into my eye was the constructor containing unreachable Code (Line 30 - and therefore unnecessary 29/31).

Yes your are right!  Thus never submit code without minimal testing and review by your own :-)

- Line 36 references ulongValue (the field) which has not been initialized so far. Afaik this is always 0 and throwing an IllegalArgumentException does neither make sense on this condition, nor will it ever happen.

Code should have been if(value < 0)

- I would not assign "value" to ulongValue in castulong (Line 45), as this might contain the negative long.

This is on purpose. Internally field ulongValue keep all the  63 bits of the long value. This  because  sub and add binary complement 2 math  doesn't care about the sign bit and will yield the good value. The bit_63 is only a field helper to help and ease logical decision related to math.

- Line 47 could be replaced by "castUlongValue.bit_63 = value < 0; Which would be easier and correct. While the current version is not. If value contains for example Long.MIN_VALUE and it is cast to int, the result will be 0.

The current version is fine but I agree that  your suggestion is much easier to read.

- The equals method only compares the ulongValue and ignores the bit_63. I might have the intention of the ulongValue interpreted incorrectly, but that seems essential for the comparison as well. As a general rule, if you implement the equals method, you should also implement hashCode.

Since the ulongValue keep internally all the 63 bits then the compares always yield the good result even if it  is signed value from Java perspective.

- The implementation of compareTo  is in my opinion a bit complex. The condition "bit_63 != compareValue.bit_63" would be much more obvious. As ulongValue could be inconsistent (either negative from the cast or positive from the constructor taking value and msb) the comparison is not always correct.

bit_63 is a boolean and you cannot use bit_63 != compareValue.bit_63 with boolean ! As for the implementation  I took my inspiration from Oracle source code Long compareTo which use a very close algorithm.  I havent take time to ponder if it can be simpler. Yes you are right the comparison doesn't yield always the correct answer. I swapped the -1 and 1 value  when I wrote the code.

- I would call the methods converting UnsignedLong to int, long, float and double casts, since they do not necessarily result in the represented value.

I agree with you and don't like the names. However these are the name requested from the Number interface  implementation that Oracle uses for their own implementation of classes Integer, Long for example.

- I did not check the correctness of the arithmetic operations, but do you have any unit tests for them?

I already found a few issues after adding some testing code. Not pretty (Could use JUnit) But this was for a fast checking of the implementation.

If I'm correct the benefit of this class could also be achieved with a simple wrapper for a long, which does not require the bit_63 boolean, as it is already available in the 63th bit of the ulongValue anyway.

I changed castulong to a static method which should provide the required benefit of long to ulong wrapper which indeed doesn't require the msb bit.

Currently, the implementation does not provide the class invariant I would expect. Beyond I think the major problem with the approach is, that there is no guarantee that the values we need to represent are only 64bit - it could be more.

I couldn't find a generic interface to help the implementation with all the needed math methods. From my point of view an interface is needed and should be defined with the needed implementation. BigInteger doesn't have one and its implementation is overkill for the need. As for invariant property you lost me. I would need some reading to better understand what that meant.  A suggestion ?

Note that adding 128 bits support in a new class shouldn't be very hard giving all the current code that exist in C for this.  Would signed 128 bits be enough ?

Thanks for your effort, but in my opinion in the current shape it is hardly possible to use this code in CDT - sorry.

Please find the included new code. I added a few methods and a few tests.  Should be much better !  Not sure if it would be useful for CDT however.
(See attached file: UnsignedLong.java)
Regards
Thomas


From: cdt-dev-bounces@xxxxxxxxxxx [mailto:cdt-dev-bounces@xxxxxxxxxxx] On Behalf Of Guy.Bonneau@xxxxxxxxxxx
Sent: Dienstag, 9. April 2013 19:43
To: CDT General developers list.
Cc: cdt-dev-bounces@xxxxxxxxxxx
Subject: Re: [cdt-dev] Bug ID 403404
 

The included class file should be very close to what is needed. It is not thoroughly tested however. I did not try to fully optimize the computing but the class should provide the functionality that is needed. It is not well commented also.  May be someone can review it and test it .If something is missing or the testing fail let me know.

Regards.
Guy(See attached file: UnsignedLong.java)

Inactive hide details for Doug Schaefer ---04/05/2013 09:37:09 AM---I think you need to look at the implementation of BigIntegeDoug Schaefer ---04/05/2013 09:37:09 AM---I think you need to look at the implementation of BigInteger before claiming it's not a performance

From: Doug Schaefer <dschaefer@xxxxxxx>
To: CDT General developers list. <cdt-dev@xxxxxxxxxxx>, David Wang <kuoweiwang@xxxxxxxxx>, Sergey Prigogin <eclipse.sprigogin@xxxxxxxxx>,
Date: 04/05/2013 09:37 AM
Subject: Re: [cdt-dev] Bug ID 403404
Sent by: cdt-dev-bounces@xxxxxxxxxxx






I think you need to look at the implementation of BigInteger before claiming it's not a performance problem. At the very least, it's really big.

It's easy to imagine a much more performant and small implementation if you know how many bits you're trying to handle. For example, with 128-bits, you can use double word algorithms are as old as I am and maximize performance.

Doug.

From: <Oberhuber>, Martin <Martin.Oberhuber@xxxxxxxxxxxxx>
Reply-To: "CDT General developers list." <cdt-dev@xxxxxxxxxxx>
Date: Friday, 5 April, 2013 8:51 AM
To: David Wang <kuoweiwang@xxxxxxxxx>, "CDT General developers list." <cdt-dev@xxxxxxxxxxx>, Sergey Prigogin <eclipse.sprigogin@xxxxxxxxx>
Subject: Re: [cdt-dev] Bug ID 403404

StackOverflow recommends BigInteger and doesn’t talk about inefficiency.

Starting with BigInteger now, it should be fairly straightforward to refactor into a different class with the same API later (ie simple search & replace on the name BigInteger and the import statements for its package). That is, only if performance measurement shows that it’s in fact a problem.

Or are you concerned that the API of BigInteger is already inefficient ? – Are you concerned about memory size or performance ?

Thanks,
Martin
--
Martin Oberhuber, SMTS / Product Architect – Development Tools, Wind River
direct +43.662.457915.85  fax +43.662.457915.6
 
DISCLAIMER: Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You. _______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/cdt-dev

DISCLAIMER: Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You. _______________________________________________
cdt-dev mailing list
cdt-dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/cdt-dev

DISCLAIMER: Privileged and/or Confidential information may be contained in this message. If you are not the addressee of this message, you may not copy, use or deliver this message to anyone. In such event, you should destroy the message and kindly notify the sender by reply e-mail. It is understood that opinions or conclusions that do not relate to the official business of the company are neither given nor endorsed by the company. Thank You.

GIF image

Back to the top