methods to write signed / unsigned integers
Hi, we have various methods to write an integer with 1, 2, 3, or 4 bytes to an img file. I always feel unsure what method to use because none of them makes clear what happens with negative values. Besides that some of the existing routines seem to throw misleading exceptions, e.g. FileBackedImgFileWriter seems to assume that it is only used for the mdr tmp file and creates texts like this: throw new MapFailedException("could not write3 to mdr tmp file"); throw new MapFailedException("could not write put3 to mdr tmp file"); Only the javadoc for put1() and put2() tell me the range (0..255 or 0..65535) . If I got that right put3() allows negative values, so I think it is NOT 0..16777215 but -8388608 .. 8388607 ? I'd like to improve the readability of the code, but I don't want to mess up anything. Would it be possible to add comments to all the methods? Gerd
Hi Gerd I started to think about this a while ago when I was fixing cities > 65535, and introduced some alternative names for get/getChar and put/putChar and also added some of the assertions. But then I noticed that some of the names I used were used for BIT I/O elsewhere, and also the Display tree shared the same code. I didn't like having code like: int n = reader.get() & 0xff; writer.putChar((char) val); inline; I think this is much clearer: int n = reader.getU1() writer.putU2(val) and allows for range checking, but you have to study the code to know if you should use putSx() or putUx() Maybe something like: getU1(), getU2(), getU3(), getUN(nBytes) getS1(), getS2(), getS3(), getSN(nBytes) putU1(int), putU2(int), putU3(int), putUN(nBytes, int) putS1(int), putS2(int), putS3(int), putSN(nBytes, int) For full integer/4 byte I/O, it is meaningless to differentiate between Signed & Unsigned, but might be clearer to have xxxU4() and xxxS4() routines as well and use instead of getInt()/putInt() There are names a bit like some of these already in imgfmt/app/ImgFile{Reader,Writer}.java and their implementations Regards Ticker On Thu, 2018-02-15 at 10:06 +0000, Gerd Petermann wrote:
Hi,
we have various methods to write an integer with 1, 2, 3, or 4 bytes to an img file. I always feel unsure what method to use because none of them makes clear what happens with negative values.
Besides that some of the existing routines seem to throw misleading exceptions, e.g. FileBackedImgFileWriter seems to assume that it is only used for the mdr tmp file and creates texts like this: throw new MapFailedException("could not write3 to mdr tmp file"); throw new MapFailedException("could not write put3 to mdr tmp file");
Only the javadoc for put1() and put2() tell me the range (0..255 or 0..65535) .
If I got that right put3() allows negative values, so I think it is NOT 0..16777215 but -8388608 .. 8388607 ?
I'd like to improve the readability of the code, but I don't want to mess up anything. Would it be possible to add comments to all the methods?
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Gerd, Ticker For ImgFileWriter I think we should just have put1(int), put2(int), put3(int) and put4(int) and remove put(byte), putChar(char) and putInt(int). So each method takes an int, so no casting is needed. There is no difference between writing unisigned and signed for any value that fits into an int. To write unsigned values greater than 2G then technically you need a long, so an unsigned version could be added as a default method on the interface put4u(long val) rather than having to implement across multiple implementations. Although I don't think it is necessary if you want to add signed/unsigned methods that range check the value, then again add them as default method on the interface. I've attached a patch that implements this. Reading is different, we do need signed/unsigned versions. I'd suggest that again have get{1,2,3,4} and remove getChar and getInt special cases, and then get1u() etc for unsigned results. All of these returning int probably, depending on what results in the best looking code. I have not done a patch for that. ..Steve
we have various methods to write an integer with 1, 2, 3, or 4 bytes to an img file. I always feel unsure what method to use because none of them makes clear what happens with negative values.
Besides that some of the existing routines seem to throw misleading exceptions, e.g. FileBackedImgFileWriter seems to assume that it is only used for the mdr tmp file and creates texts like this: throw new MapFailedException("could not write3 to mdr tmp file"); throw new MapFailedException("could not write put3 to mdr tmp file");
Only the javadoc for put1() and put2() tell me the range (0..255 or 0..65535) .
If I got that right put3() allows negative values, so I think it is NOT 0..16777215 but -8388608 .. 8388607 ?
I'd like to improve the readability of the code, but I don't want to mess up anything. Would it be possible to add comments to all the methods?
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Steve, thanks, I think that this improves the code. Small problem: I think you forgot to remove / adapt some comments in BufferedImgFileWriter, e.g. for put1 and put2. I'd prefer to replace them by an @override annotation. Gerd ________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Steve Ratcliffe <steve@parabola.me.uk> Gesendet: Sonntag, 18. Februar 2018 22:37:19 An: mkgmap-dev@lists.mkgmap.org.uk Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers Hi Gerd, Ticker For ImgFileWriter I think we should just have put1(int), put2(int), put3(int) and put4(int) and remove put(byte), putChar(char) and putInt(int). So each method takes an int, so no casting is needed. There is no difference between writing unisigned and signed for any value that fits into an int. To write unsigned values greater than 2G then technically you need a long, so an unsigned version could be added as a default method on the interface put4u(long val) rather than having to implement across multiple implementations. Although I don't think it is necessary if you want to add signed/unsigned methods that range check the value, then again add them as default method on the interface. I've attached a patch that implements this. Reading is different, we do need signed/unsigned versions. I'd suggest that again have get{1,2,3,4} and remove getChar and getInt special cases, and then get1u() etc for unsigned results. All of these returning int probably, depending on what results in the best looking code. I have not done a patch for that. ..Steve
we have various methods to write an integer with 1, 2, 3, or 4 bytes to an img file. I always feel unsure what method to use because none of them makes clear what happens with negative values.
Besides that some of the existing routines seem to throw misleading exceptions, e.g. FileBackedImgFileWriter seems to assume that it is only used for the mdr tmp file and creates texts like this: throw new MapFailedException("could not write3 to mdr tmp file"); throw new MapFailedException("could not write put3 to mdr tmp file");
Only the javadoc for put1() and put2() tell me the range (0..255 or 0..65535) .
If I got that right put3() allows negative values, so I think it is NOT 0..16777215 but -8388608 .. 8388607 ?
I'd like to improve the readability of the code, but I don't want to mess up anything. Would it be possible to add comments to all the methods?
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi I think it is important to indicate if a signed or unsigned is being written so that the range can be asserted: eg: public void putU1(int val) { assert val >= 0 && val <= 255 : val; buf.put((byte)val); } and public void putS1(int val) { assert val >= -128 && val <= 127 : val; buf.put((byte)val); } Ticker On Sun, 2018-02-18 at 21:37 +0000, Steve Ratcliffe wrote:
Hi Gerd, Ticker
For ImgFileWriter I think we should just have put1(int), put2(int), put3(int) and put4(int) and remove put(byte), putChar(char) and putInt(int).
So each method takes an int, so no casting is needed. There is no difference between writing unisigned and signed for any value that fits into an int.
To write unsigned values greater than 2G then technically you need a long, so an unsigned version could be added as a default method on the interface put4u(long val) rather than having to implement across multiple implementations.
Although I don't think it is necessary if you want to add signed/unsigned methods that range check the value, then again add them as default method on the interface.
I've attached a patch that implements this.
Reading is different, we do need signed/unsigned versions. I'd suggest that again have get{1,2,3,4} and remove getChar and getInt special cases, and then get1u() etc for unsigned results. All of these returning int probably, depending on what results in the best looking code. I have not done a patch for that.
..Steve
we have various methods to write an integer with 1, 2, 3, or 4 bytes to an img file. I always feel unsure what method to use because none of them makes clear what happens with negative values.
Besides that some of the existing routines seem to throw misleading exceptions, e.g. FileBackedImgFileWriter seems to assume that it is only used for the mdr tmp file and creates texts like this: throw new MapFailedException("could not write3 to mdr tmp file"); throw new MapFailedException("could not write put3 to mdr tmp file");
Only the javadoc for put1() and put2() tell me the range (0..255 or 0..65535) .
If I got that right put3() allows negative values, so I think it is NOT 0..16777215 but -8388608 .. 8388607 ?
I'd like to improve the readability of the code, but I don't want to mess up anything. Would it be possible to add comments to all the methods?
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Ticker, please explain why you think that this is important. I like the idea that I don't have to care about the write method, I only have to make sure that the fields can hold the value ranges. In most cases we can use int even if IMG format uses a single byte, since we don't have huge amounts of header structures. Gerd ________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Ticker Berkin <rwb-mkgmap@jagit.co.uk> Gesendet: Montag, 19. Februar 2018 10:19:56 An: Development list for mkgmap Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers Hi I think it is important to indicate if a signed or unsigned is being written so that the range can be asserted: eg: public void putU1(int val) { assert val >= 0 && val <= 255 : val; buf.put((byte)val); } and public void putS1(int val) { assert val >= -128 && val <= 127 : val; buf.put((byte)val); } Ticker On Sun, 2018-02-18 at 21:37 +0000, Steve Ratcliffe wrote:
Hi Gerd, Ticker
For ImgFileWriter I think we should just have put1(int), put2(int), put3(int) and put4(int) and remove put(byte), putChar(char) and putInt(int).
So each method takes an int, so no casting is needed. There is no difference between writing unisigned and signed for any value that fits into an int.
To write unsigned values greater than 2G then technically you need a long, so an unsigned version could be added as a default method on the interface put4u(long val) rather than having to implement across multiple implementations.
Although I don't think it is necessary if you want to add signed/unsigned methods that range check the value, then again add them as default method on the interface.
I've attached a patch that implements this.
Reading is different, we do need signed/unsigned versions. I'd suggest that again have get{1,2,3,4} and remove getChar and getInt special cases, and then get1u() etc for unsigned results. All of these returning int probably, depending on what results in the best looking code. I have not done a patch for that.
..Steve
we have various methods to write an integer with 1, 2, 3, or 4 bytes to an img file. I always feel unsure what method to use because none of them makes clear what happens with negative values.
Besides that some of the existing routines seem to throw misleading exceptions, e.g. FileBackedImgFileWriter seems to assume that it is only used for the mdr tmp file and creates texts like this: throw new MapFailedException("could not write3 to mdr tmp file"); throw new MapFailedException("could not write put3 to mdr tmp file");
Only the javadoc for put1() and put2() tell me the range (0..255 or 0..65535) .
If I got that right put3() allows negative values, so I think it is NOT 0..16777215 but -8388608 .. 8388607 ?
I'd like to improve the readability of the code, but I don't want to mess up anything. Would it be possible to add comments to all the methods?
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Gerd The final bit patterns written into the IMG will be the same regardless of using putUx or putSx, but the Garmin device or map renderer will interpret each as either signed or unsigned, depending on where it is in the IMG. If we write, say a value of 200 into a byte, where this is interpreted as a signed by the device, we just have a possibly difficult-to-diagnose map failure. Similarly writing, say, -1, to something that will be interpreted as unsigned will cause a problem. Having different methods with relevant assertions allows these errors to be caught early on. It also clarifies the code. Given that nearly all the int writes in imgfmt are going to be changed, it seems sensible to do it as best as possible. Also, in imgfmt, there is code to read back the IMG structures. This has to known if it should use signed or unsigned read methods. Often, quite close in the code, there will be the output calls for the same items, so having: someFld = reader.get1S(); ...; writer.put1S(someFld) looks difficult to argue with. I don't understand your comment: "... we can use int even if IMG format uses a single byte, since we don't have huge amounts of header structures." It is only during the method call (putxx()) or return (getxx()) that ints will be in use rather than byte/char Regards Ticker On Tue, 2018-02-20 at 09:15 +0000, Gerd Petermann wrote:
Hi Ticker,
please explain why you think that this is important. I like the idea that I don't have to care about the write method, I only have to make sure that the fields can hold the value ranges. In most cases we can use int even if IMG format uses a single byte, since we don't have huge amounts of header structures.
Gerd
________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Ticker Berkin <rwb-mkgmap@jagit.co.uk> Gesendet: Montag, 19. Februar 2018 10:19:56 An: Development list for mkgmap Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers
Hi
I think it is important to indicate if a signed or unsigned is being written so that the range can be asserted:
eg: public void putU1(int val) { assert val >= 0 && val <= 255 : val; buf.put((byte)val); } and public void putS1(int val) { assert val >= -128 && val <= 127 : val; buf.put((byte)val); }
Ticker
On Sun, 2018-02-18 at 21:37 +0000, Steve Ratcliffe wrote:
Hi Gerd, Ticker
For ImgFileWriter I think we should just have put1(int), put2(int), put3(int) and put4(int) and remove put(byte), putChar(char) and putInt(int).
So each method takes an int, so no casting is needed. There is no difference between writing unisigned and signed for any value that fits into an int.
To write unsigned values greater than 2G then technically you need a long, so an unsigned version could be added as a default method on the interface put4u(long val) rather than having to implement across multiple implementations.
Although I don't think it is necessary if you want to add signed/unsigned methods that range check the value, then again add them as default method on the interface.
I've attached a patch that implements this.
Reading is different, we do need signed/unsigned versions. I'd suggest that again have get{1,2,3,4} and remove getChar and getInt special cases, and then get1u() etc for unsigned results. All of these returning int probably, depending on what results in the best looking code. I have not done a patch for that.
..Steve
we have various methods to write an integer with 1, 2, 3, or 4 bytes to an img file. I always feel unsure what method to use because none of them makes clear what happens with negative values.
Besides that some of the existing routines seem to throw misleading exceptions, e.g. FileBackedImgFileWriter seems to assume that it is only used for the mdr tmp file and creates texts like this: throw new MapFailedException("could not write3 to mdr tmp file"); throw new MapFailedException("could not write put3 to mdr tmp file");
Only the javadoc for put1() and put2() tell me the range (0..255 or 0..65535) .
If I got that right put3() allows negative values, so I think it is NOT 0..16777215 but -8388608 .. 8388607 ?
I'd like to improve the readability of the code, but I don't want to mess up anything. Would it be possible to add comments to all the methods?
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev _______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi, okay, I think Ticker has some good points here. Reg. my comment "... we can use int even if IMG format uses a single byte, since we don't have huge amounts of header structures." : You can ignore this, I thought about using an int to store the value of a field that is written into e.g. 2 bytes. We often do this because Java returns int when doing calculations e.g. short someFld = 10; int x = 2; someFld /= x; // works someFld = someFld / x; // doesn't compile someFld = (short) (someFld / x); // ok Therefore I prefer to use type int for someFld when I use it for calculations. The explicit type casts are always causing trouble when you change a field from short to int later. Gerd ________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Ticker Berkin <rwb-mkgmap@jagit.co.uk> Gesendet: Dienstag, 20. Februar 2018 11:52:11 An: mkgmap-dev@lists.mkgmap.org.uk Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers Hi Gerd The final bit patterns written into the IMG will be the same regardless of using putUx or putSx, but the Garmin device or map renderer will interpret each as either signed or unsigned, depending on where it is in the IMG. If we write, say a value of 200 into a byte, where this is interpreted as a signed by the device, we just have a possibly difficult-to-diagnose map failure. Similarly writing, say, -1, to something that will be interpreted as unsigned will cause a problem. Having different methods with relevant assertions allows these errors to be caught early on. It also clarifies the code. Given that nearly all the int writes in imgfmt are going to be changed, it seems sensible to do it as best as possible. Also, in imgfmt, there is code to read back the IMG structures. This has to known if it should use signed or unsigned read methods. Often, quite close in the code, there will be the output calls for the same items, so having: someFld = reader.get1S(); ...; writer.put1S(someFld) looks difficult to argue with. I don't understand your comment: "... we can use int even if IMG format uses a single byte, since we don't have huge amounts of header structures." It is only during the method call (putxx()) or return (getxx()) that ints will be in use rather than byte/char Regards Ticker On Tue, 2018-02-20 at 09:15 +0000, Gerd Petermann wrote:
Hi Ticker,
please explain why you think that this is important. I like the idea that I don't have to care about the write method, I only have to make sure that the fields can hold the value ranges. In most cases we can use int even if IMG format uses a single byte, since we don't have huge amounts of header structures.
Gerd
________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Ticker Berkin <rwb-mkgmap@jagit.co.uk> Gesendet: Montag, 19. Februar 2018 10:19:56 An: Development list for mkgmap Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers
Hi
I think it is important to indicate if a signed or unsigned is being written so that the range can be asserted:
eg: public void putU1(int val) { assert val >= 0 && val <= 255 : val; buf.put((byte)val); } and public void putS1(int val) { assert val >= -128 && val <= 127 : val; buf.put((byte)val); }
Ticker
On Sun, 2018-02-18 at 21:37 +0000, Steve Ratcliffe wrote:
Hi Gerd, Ticker
For ImgFileWriter I think we should just have put1(int), put2(int), put3(int) and put4(int) and remove put(byte), putChar(char) and putInt(int).
So each method takes an int, so no casting is needed. There is no difference between writing unisigned and signed for any value that fits into an int.
To write unsigned values greater than 2G then technically you need a long, so an unsigned version could be added as a default method on the interface put4u(long val) rather than having to implement across multiple implementations.
Although I don't think it is necessary if you want to add signed/unsigned methods that range check the value, then again add them as default method on the interface.
I've attached a patch that implements this.
Reading is different, we do need signed/unsigned versions. I'd suggest that again have get{1,2,3,4} and remove getChar and getInt special cases, and then get1u() etc for unsigned results. All of these returning int probably, depending on what results in the best looking code. I have not done a patch for that.
..Steve
we have various methods to write an integer with 1, 2, 3, or 4 bytes to an img file. I always feel unsure what method to use because none of them makes clear what happens with negative values.
Besides that some of the existing routines seem to throw misleading exceptions, e.g. FileBackedImgFileWriter seems to assume that it is only used for the mdr tmp file and creates texts like this: throw new MapFailedException("could not write3 to mdr tmp file"); throw new MapFailedException("could not write put3 to mdr tmp file");
Only the javadoc for put1() and put2() tell me the range (0..255 or 0..65535) .
If I got that right put3() allows negative values, so I think it is NOT 0..16777215 but -8388608 .. 8388607 ?
I'd like to improve the readability of the code, but I don't want to mess up anything. Would it be possible to add comments to all the methods?
Gerd
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev _______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi And here is the patch for just the reading side. I decided that it was best to leave in 'byte get()' as there are many places where a byte is used to hold the result and a cast would be needed for 'int get1()'. The tests pass, but there is more scope for getting this patch wrong so needs some good testing. Also display project would have to be patched to match. ..Steve
Hi Steve, I think msg.patch looks good. I'd like to postpone the other changes until the "shrink factor" in DEM is understood. Gerd ________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Steve Ratcliffe <steve@parabola.me.uk> Gesendet: Mittwoch, 21. Februar 2018 23:40:35 An: mkgmap-dev@lists.mkgmap.org.uk Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers Hi And here is the patch for just the reading side. I decided that it was best to leave in 'byte get()' as there are many places where a byte is used to hold the result and a cast would be needed for 'int get1()'. The tests pass, but there is more scope for getting this patch wrong so needs some good testing. Also display project would have to be patched to match. ..Steve
Hi Gerd / Steve Starting from Steve's patches (getting.patch, msg.patch and img -write.patch), I've changed mkgmap+test to have/use: int get1s(), get2s(), get3s(), get4() ing get1u(), get2u(), get3u(), getNu() put1s(int), put2s(int), put3s(int), put4(int) put1u(int), put2u(int), put3u(int), putNu(nBytes, int) throughout almost all of imgfmt. putX{s/u} assert the correct range and the getX{s/u} sign-extend or not as appropriate. assert and sign-extend are meaningless for get4()/put4() so it doesn't have the s/u variants. In a lot of places I've changed the working variables from byte/char/short to int and avoided any premature range narrowing. There are a couple of places where I've left byte get() and put(byte) because bit fiddling makes it meaningless to consider the value as a number or I didn't want to touch delicate logic, but generally flags are handled as ints. I had some problems with test/func/files/GmapsuppTest.java where an empty map leads to negative subdivision width/height and lat/long values bigger than 3 bytes but I've dealt with this. Something that confused me in imgfmt/app/trergn/TREFileReader.java, around line 118, was the 2*width and 2*height in new SubdivData(... As far as I can see these values have just been read from an .img and so should be written back exactly as they were. If/when you are interested, I'll send the patch. In the meantime I'm running with it to see if there are any problems I have a similar patch for Display Ticker On Fri, 2018-02-23 at 08:03 +0000, Gerd Petermann wrote:
Hi Steve,
I think msg.patch looks good. I'd like to postpone the other changes until the "shrink factor" in DEM is understood.
Gerd
________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Steve Ratcliffe <steve@parabola.me.uk> Gesendet: Mittwoch, 21. Februar 2018 23:40:35 An: mkgmap-dev@lists.mkgmap.org.uk Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers
Hi
And here is the patch for just the reading side.
I decided that it was best to leave in 'byte get()' as there are many places where a byte is used to hold the result and a cast would be needed for 'int get1()'.
The tests pass, but there is more scope for getting this patch wrong so needs some good testing.
Also display project would have to be patched to match.
..Steve
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Ticker Sounds good, yes please send the patch. Cheers Steve
Starting from Steve's patches (getting.patch, msg.patch and img -write.patch), I've changed mkgmap+test to have/use: int get1s(), get2s(), get3s(), get4() ing get1u(), get2u(), get3u(), getNu() put1s(int), put2s(int), put3s(int), put4(int) put1u(int), put2u(int), put3u(int), putNu(nBytes, int) throughout almost all of imgfmt.
putX{s/u} assert the correct range and the getX{s/u} sign-extend or not as appropriate. assert and sign-extend are meaningless for get4()/put4() so it doesn't have the s/u variants.
In a lot of places I've changed the working variables from byte/char/short to int and avoided any premature range narrowing.
There are a couple of places where I've left byte get() and put(byte) because bit fiddling makes it meaningless to consider the value as a number or I didn't want to touch delicate logic, but generally flags are handled as ints.
I had some problems with test/func/files/GmapsuppTest.java where an empty map leads to negative subdivision width/height and lat/long values bigger than 3 bytes but I've dealt with this.
Something that confused me in imgfmt/app/trergn/TREFileReader.java, around line 118, was the 2*width and 2*height in new SubdivData(... As far as I can see these values have just been read from an .img and so should be written back exactly as they were.
If/when you are interested, I'll send the patch. In the meantime I'm running with it to see if there are any problems
I have a similar patch for Display
Ticker
Hi Steve / Gerd Here is the main patch Ticker On Tue, 2018-03-06 at 23:02 +0000, Steve Ratcliffe wrote:
Hi Ticker
Sounds good, yes please send the patch.
Cheers Steve
Starting from Steve's patches (getting.patch, msg.patch and img -write.patch), I've changed mkgmap+test to have/use: int get1s(), get2s(), get3s(), get4() ing get1u(), get2u(), get3u(), getNu() put1s(int), put2s(int), put3s(int), put4(int) put1u(int), put2u(int), put3u(int), putNu(nBytes, int) throughout almost all of imgfmt.
putX{s/u} assert the correct range and the getX{s/u} sign-extend or not as appropriate. assert and sign-extend are meaningless for get4()/put4() so it doesn't have the s/u variants.
In a lot of places I've changed the working variables from byte/char/short to int and avoided any premature range narrowing.
There are a couple of places where I've left byte get() and put(byte) because bit fiddling makes it meaningless to consider the value as a number or I didn't want to touch delicate logic, but generally flags are handled as ints.
I had some problems with test/func/files/GmapsuppTest.java where an empty map leads to negative subdivision width/height and lat/long values bigger than 3 bytes but I've dealt with this.
Something that confused me in imgfmt/app/trergn/TREFileReader.java, around line 118, was the 2*width and 2*height in new SubdivData(... As far as I can see these values have just been read from an .img and so should be written back exactly as they were.
If/when you are interested, I'll send the patch. In the meantime I'm running with it to see if there are any problems
I have a similar patch for Display
Ticker
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Ticker, sorry, did not find the time to look at it until now. At least for DEM this would not be correct, some values should be written with the signed put versions. I've attached a corrected version of the patch. Besides that I'd prefer not to see comments like // don't think needed: For further testing I'd need the patch for display tool as well. Gerd ________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Ticker Berkin <ticker@jagIT.co.uk> Gesendet: Mittwoch, 7. März 2018 10:00:46 An: Development list for mkgmap Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers Hi Steve / Gerd Here is the main patch Ticker On Tue, 2018-03-06 at 23:02 +0000, Steve Ratcliffe wrote:
Hi Ticker
Sounds good, yes please send the patch.
Cheers Steve
Starting from Steve's patches (getting.patch, msg.patch and img -write.patch), I've changed mkgmap+test to have/use: int get1s(), get2s(), get3s(), get4() ing get1u(), get2u(), get3u(), getNu() put1s(int), put2s(int), put3s(int), put4(int) put1u(int), put2u(int), put3u(int), putNu(nBytes, int) throughout almost all of imgfmt.
putX{s/u} assert the correct range and the getX{s/u} sign-extend or not as appropriate. assert and sign-extend are meaningless for get4()/put4() so it doesn't have the s/u variants.
In a lot of places I've changed the working variables from byte/char/short to int and avoided any premature range narrowing.
There are a couple of places where I've left byte get() and put(byte) because bit fiddling makes it meaningless to consider the value as a number or I didn't want to touch delicate logic, but generally flags are handled as ints.
I had some problems with test/func/files/GmapsuppTest.java where an empty map leads to negative subdivision width/height and lat/long values bigger than 3 bytes but I've dealt with this.
Something that confused me in imgfmt/app/trergn/TREFileReader.java, around line 118, was the 2*width and 2*height in new SubdivData(... As far as I can see these values have just been read from an .img and so should be written back exactly as they were.
If/when you are interested, I'll send the patch. In the meantime I'm running with it to see if there are any problems
I have a similar patch for Display
Ticker
_______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Gerd I've attached the Display patch - this was a quick hack global edit and could be improved by using the relevant signed/unsigned get methods instead of masking... I'm happy to do this - let me know. Yes to DEM changes. Also the nearby asserts can go now because put2s() will check the range. // don't think needed: could be changed to // not needed at moment: but I think it is worth leaving these methods (getNs()/putNs()) commented in the ImgFile interface source. Ticker On Thu, 2018-03-15 at 10:12 +0000, Gerd Petermann wrote:
Hi Ticker,
sorry, did not find the time to look at it until now. At least for DEM this would not be correct, some values should be written with the signed put versions. I've attached a corrected version of the patch. Besides that I'd prefer not to see comments like // don't think needed:
For further testing I'd need the patch for display tool as well.
Gerd
Hi Ticker, yes, I think display tool needs more work. You commented a I think the method names in Displayer.java should be changed so that they are similar to the corresponding reader methods. Example: public int int3Value(String text) would be public int int3uValue(String text) I see some places where you replaced item.setBytes(reader.getChar()); by item.setBytes((char)reader.get2u()); I think the cast to char should be removed. I also see code like this: c = item.setBytes(reader.get2u()) & 0xffff; I think the & 0xffff is obsolete and confusing. I am pretty sure that some lines in MdrDisplay are wrong (not because of your patch). I think all places where get3s() is used need a closer look. We might have to change them to get3u(). For example: int record = reader.get3s(); off = item.setBytes3(reader.get3s()); Record numbers and offsets are normally not negative. I'll have a closer look today. I also agree that this snippet from NodConvert looks wrong: } else if (restrbytes == 3) { size = reader.get2u() & 0xffffff; // %%% think mistake I've not used NodConvert for a long time, it might well be behind the last findings reg. NOD. Gerd ________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Ticker Berkin <ticker@jagIT.co.uk> Gesendet: Donnerstag, 15. März 2018 15:21:25 An: mkgmap-dev@lists.mkgmap.org.uk Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers Hi Gerd I've attached the Display patch - this was a quick hack global edit and could be improved by using the relevant signed/unsigned get methods instead of masking... I'm happy to do this - let me know. Yes to DEM changes. Also the nearby asserts can go now because put2s() will check the range. // don't think needed: could be changed to // not needed at moment: but I think it is worth leaving these methods (getNs()/putNs()) commented in the ImgFile interface source. Ticker On Thu, 2018-03-15 at 10:12 +0000, Gerd Petermann wrote:
Hi Ticker,
sorry, did not find the time to look at it until now. At least for DEM this would not be correct, some values should be written with the signed put versions. I've attached a corrected version of the patch. Besides that I'd prefer not to see comments like // don't think needed:
For further testing I'd need the patch for display tool as well.
Gerd
Hi Gerd Looking at the Displayer and the DisplayItem methods, I think it would be quite a lot of work for little benefit to change it all to have the various 1/2/3/4 s/u type methods. It would need all the variants on setBytes as well. Unless we introduce differently named setBytes() methods, we still need the casts as in item.setBytes((char)reader.get2u()) All the redundant masking should be removed Also any obvious uses of signed when should be unsigned get, along the other errors that have been noticed I'll can do another pass over the code with these changes and send you a combined path if you want, but I feel it would be simpler and saver to apply the current patch at the same time as you apply the main mkgmap patch and make these changes as a new version Ticker On Fri, 2018-03-16 at 08:22 +0000, Gerd Petermann wrote:
Hi Ticker,
yes, I think display tool needs more work. You commented a I think the method names in Displayer.java should be changed so that they are similar to the corresponding reader methods. Example: public int int3Value(String text) would be public int int3uValue(String text)
I see some places where you replaced item.setBytes(reader.getChar()); by item.setBytes((char)reader.get2u()); I think the cast to char should be removed.
I also see code like this: c = item.setBytes(reader.get2u()) & 0xffff; I think the & 0xffff is obsolete and confusing.
I am pretty sure that some lines in MdrDisplay are wrong (not because of your patch). I think all places where get3s() is used need a closer look. We might have to change them to get3u(). For example: int record = reader.get3s(); off = item.setBytes3(reader.get3s()); Record numbers and offsets are normally not negative. I'll have a closer look today.
I also agree that this snippet from NodConvert looks wrong: } else if (restrbytes == 3) { size = reader.get2u() & 0xffffff; // %%% think mistake
I've not used NodConvert for a long time, it might well be behind the last findings reg. NOD.
Gerd
________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Ticker Berkin <ticker@jagIT.co.uk> Gesendet: Donnerstag, 15. März 2018 15:21:25 An: mkgmap-dev@lists.mkgmap.org.uk Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers
Hi Gerd
I've attached the Display patch - this was a quick hack global edit and could be improved by using the relevant signed/unsigned get methods instead of masking... I'm happy to do this - let me know.
Yes to DEM changes. Also the nearby asserts can go now because put2s() will check the range.
// don't think needed: could be changed to // not needed at moment: but I think it is worth leaving these methods (getNs()/putNs()) commented in the ImgFile interface source.
Ticker
On Thu, 2018-03-15 at 10:12 +0000, Gerd Petermann wrote:
Hi Ticker,
sorry, did not find the time to look at it until now. At least for DEM this would not be correct, some values should be written with the signed put versions. I've attached a corrected version of the patch. Besides that I'd prefer not to see comments like // don't think needed:
For further testing I'd need the patch for display tool as well.
Gerd
mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Gerd I was hoping you'd apply/commit img_io_3.patch (15-mar, from you) and the display_io_1.patch (15-mar, from me) and then I'll do another global edit and tidy-up on Display as per the posts on 16-mar. Regards Ticker
Hi Ticker, sorry for that. I waited for a comment from Steve and forgot about it. I'd still prefer that Steve gives an OK because he wrote most of the code that the patch changes. Gerd ________________________________________ Von: mkgmap-dev <mkgmap-dev-bounces@lists.mkgmap.org.uk> im Auftrag von Ticker Berkin <rwb-mkgmap@jagit.co.uk> Gesendet: Dienstag, 17. April 2018 12:51:36 An: mkgmap development Betreff: Re: [mkgmap-dev] methods to write signed / unsigned integers Hi Gerd I was hoping you'd apply/commit img_io_3.patch (15-mar, from you) and the display_io_1.patch (15-mar, from me) and then I'll do another global edit and tidy-up on Display as per the posts on 16-mar. Regards Ticker _______________________________________________ mkgmap-dev mailing list mkgmap-dev@lists.mkgmap.org.uk http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Hi Gerd, Ticker
sorry for that. I waited for a comment from Steve and forgot about it. I'd still prefer that Steve gives an OK because he wrote most of the code that the patch changes.
I read through the patch and done some very simple tests and it looks OK to me. Hope someone has tested some really big maps. I think we should remove the commented out code and I noticed a few unneeded casts remaining (some maybe not actually as a result of the patch), and some javadoc errors. I've attached an incremental patch of the things I did, which you can take or leave as you wish. Lets get it committed. Its a big patch, thanks for going through all the cases and completing it. Steve
participants (5)
-
Gerd Petermann -
Gerd Petermann
-
Steve Ratcliffe -
Ticker Berkin -
Ticker Berkin