Hi Andrew, > On Jun 5, 2019, at 10:10 AM, Andrew Dinn <ad...@redhat.com> wrote: > > Thanks very much for reviewing the patch. Also, commendations on your > eagle-eyed thoroughness.
Thanks. :-) > I have happily accepted most corrections. I have commented (inline > below) Likewise. > in a few places where I think things might need to be discussed > further. I will upload a new webrev after these points are resolved. OK > On 05/06/2019 15:36, Brian Burkhalter wrote: >> I have some minor comments on webrev.05. (webrev.06 looks rather strange.) > > That is the next working version being queued up -- you may well have > seen an intermediate version while it was uploading. Must have done as it looks OK now. >> MappedByteBuffer.java >> 84:“any of other modes” -> “any of the other modes”, “this” -> “This” >> 85:nit: usually use American spelling: “behaviour” -> “behavior” (but it >> does not matter so much here as it is not public javadoc. >> 172:“pasing” -> “passing” >> 175:delete “using” >> 181:comma or semicolon before “otherwise” > > All done. I even gritted my teeth and spelled behaviour 'US-wize’. ;-) >> 959:Is there a possibility of overflow, e.g., use Math.addExact(address, >> length) ? > > I think it assumed that all OSes currently ported (certainly Linux) will > never map a vmem region so that addition of a valid internal offset > might overflow. If it did we would be in big trouble as regards null > pointer checking. So, modulo input from those wiser than me in operating > system lore, I think this is not needed. It sounds reasonable to leave it as-is in that case. >> 98-110:Put symbolic name definitions at head of file? > > I thought these definitions would be better placed at the point where > the defines are needed so as to make it clear why this is being done. If > you are strongly motivated to move them to the more normal location I > will do so. I think that is reasonable as well so no need to move them. >> FileChannelImpl.c (Windows) >> 91:Use same message as at Unix version line 135? > > Perhaps but I don't think they are the same error and thougt it beter to > reinforce that with different messages. Note the different exception > types. In the case of Windows we should never reach this point -- at > least not until a Windows port comes along and requires the message to > be changed (effectively removing here and replacing it with somewhere > else). So, an InternalError is thrown. In the Unix case the error might > conceivably happen (because the target OS mmap implementation does not > have proper MAP_SYNC support. Hence the use of IOException. All right, then leave it as it is. >> ExtendedMapMode >> 13:Constant name should be MAP_MODE_CONSTRUCTOR. >> 13:Insert blank line after >> 34-36:Move to before line 13. >> 24:Move constructor to end of class. > Yes, all good. Thanks! Good. I suppose these changes will be in version 7. If it is likely that comments will be forthcoming from others then it might be worth waiting to incorporate any further changes in version 7. Thanks, Brian
