search

nyholku / purejavacomm

Pure Java implementation of JavaComm SerialPort
http://www.sparetimelabs.com/purejavacomm/index.html
BSD 3-Clause "New" or "Revised" License
362 stars 146 forks source link

Push request branch 1 #119

Closed rdiez closed 3 years ago

rdiez commented 5 years ago

This is my first pull request. Let's see if we can get such pull requests going.

What's the tab size you are using? I realised that the source is using tabs after my first commit attempt. I would actually prefer spaces.

I tried to fix some "Synchronization on non-final field" warnings too, but it turns out that you cannot have a variable marked as 'volatile' and 'final' at the same time.

I wonder whether those 'volatile' are really necessary. There is another NetBeans warning titled "volatile array field" on the same source code that suggests 'volatile' has no real impact.

nyholku commented 5 years ago

Hi,

thanks for PR.

The @Overrides are welcome but I don't want to introduce asserts into the code base, at least not at this point in time, not while we are working on something else, not in some random source file causing inconsistency. So let's leave that aside for now.

I will cherrypick the @Overrides over the weekend.

As to my tab size, that is 4.

Yes, there maybe a few superfluous volatiles but they are benign and it is better to be safe than sorry (having a field that should be volatile but is not can be a real pain to track down cause the problem may manifest itself only on some systems, with particular JVM, at rare and random time).

Lets concentrate on the real issues you (others) are having.

luca-domenichini commented 5 years ago

Quick tip: not just "asserts" but there is a "throw" added to line 530 too, and it was probably missing in the master branch. But I would do another PR for that fix..

rdiez commented 5 years ago

I think not including the asserts is a mistake. In my experience, they tend to help. They should not interfere with normal usage, for asserts are not normally enabled in the JVM.

Could you at least take the other changes in that commit? The new/reworked comments should help future coders, and the missing 'throw' is important.

nyholku commented 5 years ago

On 11 Jan 2019, at 10.19, rdiez notifications@github.com wrote:

I think not including the asserts is a mistake. In my experience, they tend to help. They should not interfere with normal usage, for asserts are not normally enabled in the JVM.

Could you at least take the other changes in that commit? The new/reworked comments should help future coders, and the missing 'throw' is important.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

Sure, apart from the assert, I will pull the other changes after I've reviewed them (should be trivial).

In principle I like asserts though there are other views if you google around.

Regardless, this is not the time and place to include them at two happenstance places in the code and to start suddenly using them. As to weather asserts are enabled in 'normal' code, that we do not know. As the current trend and recommendation is to include JRE with the application it may well be enabled by the developer for their application for any number of reasons and the end user may have no control over that and thus the assert can change behaviour to the worse for some people.

wbr Kusti

swarwick commented 5 years ago

I believe that missing throw would have helped with my situation before we changed to FTDI chipset, so good catch.