search

Praqma / memory-map-plugin

A repository for the memory-map-plugin
13 stars 16 forks source link

Memory-map-plugin broken when parameter "Word size" is other than 16 #40

Closed flreinhard closed 6 years ago

flreinhard commented 6 years ago

Steps to reproduce:

Reason: HexUtils.java, function wordCount. The code changed from return (HexUtils.getRadix(hexString, HEXA_RADIX)) / HexUtils.scale.get(scale.toLowerCase()); which is correct, since HEXA_RADIX is 16 and the function decodes a hex-string, to return HexUtils.getRadix(hexString, wordcount) / (double) HexUtils.scale.get(scale.toLowerCase()); which is wrong, but does not immediate cause an exception. If your hex string ix 0x10 and your "Word count" is 8, there will be no exception but the result is wrong. The decimal representation of 0x10 will be calculated as 8 instead of 16! If your hex string is 0xF0 and your "Word count" is 8, you will get an exception java.lang.NumberFormatException.

I guess supplying a patch does not make sense, since it's just a revert of that line an reintroduce the HEXA_RADIX constant.

flreinhard commented 6 years ago

The Pullrequest PR #37 from august last year also fixes the issue and adds further tests.

buep commented 6 years ago

Hi @flreinhard

Thanks for the inputs, and sorry for not expediting the PR earlier.

I have pulled in your changes in PR #37 on a branch for this issue, only changing the hard coded 16 to HEX_RADIX constant.

But the wordsize parameter was unused, so I fixed that up as well.

Please review, and I'll see if I can get it releases soon.

https://github.com/Praqma/memory-map-plugin/compare/40-memory-map-plugin-broken-when-parameter-word-size-is-other-than-16

flreinhard commented 6 years ago

Looks good except for: Revert HexUtilsTest.java, Chunk @@ -43,13 +43,21 @@ Does not make sense anymore, since you dropped the wordSize parameter for HexUtils.wordCount.

Note: that was not my pullrequest, credit goes to @AndersHoglund

buep commented 6 years ago

Yes I dropped the wordsize and that was the essence of suggested edits and it ended up with the function not using it.

It's not my own code and I didn't go through all details, so I'm happy to spend a little more time on it if you think the combined change is not correct ?

flreinhard commented 6 years ago

As i wrote: just revert the Chunk "HexUtilsTest.java, Chunk @@ -43,13 +43,21 @@" https://github.com/Praqma/memory-map-plugin/compare/40-memory-map-plugin-broken-when-parameter-word-size-is-other-than-16#diff-5b546fd26882740174b43035011b917d

The result of the combined change is, that you run the same tests 3 times. This does not make sense ;)

Aresius423 commented 6 years ago

Hey,

I don't want to unnecessarily create a PR for this, but would it be possible to delete HexUtilsTest.java:53-60 and restore the previous comment on line 46, so you can close this issue and release the fix from PR #37?

buep commented 6 years ago

Yes - we will put progress on fixing what is needed now.

I'm sorry for the delay, I had to less time so my colleague will try to help and look over it.

OEHC commented 6 years ago

It surely does not make sense to run the same set of tests multiple times. I have deleted/reverted the section you mentioned @flreinhard, @Aresius423. This should make the code sound and ready for release.