Hi, here is the updated webrev:
http://cr.openjdk.java.net/~dbuck/4906983.3/ Hope the comments are fine now. What would be the normal procedure in net-dev for accepting a change (a 2 group-member thumbs-up like in core-libs)? -- Sebastian Am 14.09.2015 um 10:53 schrieb Chris Hegarty: > On 13/09/15 20:18, Sebastian Sickelmann wrote: >> Am 13.09.2015 um 16:25 schrieb Chris Hegarty: >>> >>>> On 13 Sep 2015, at 14:07, Mark Sheppard >>>> <<mailto:mark.shepp...@oracle.com>mark.shepp...@oracle.com> wrote: >>>> >>>> Hi >>>> I don't think the URL string http://server:-1/path can be >>>> considered a valid URL as the port value -1 is not legal port value. >>> >>> I agree, but java.net <http://java.net>.URL is a legacy API. The only >>> changes we can realistically make to it now are to tighten the spec as >>> per existing behaviour ( in a manner so as to not encourage it to be >>> used in an incorrect way ). >> >> I think we could change the behavior of the constructors which receive a >> "spec" parameter. In non of those javadocs the -1 is mentioned. ] > > I would be opposed to changing the behavior of any of these constructors. > > > But this >> is maybe another discussion, unrelated to cleanup the javadoc. I would >> start it in another thread, when you agree to discuss this separatly. > > Separately, if at all. > >> For now i would leave the testcase and add a longer comment to it. > > For now, in the test, just remove the string spec constructors, and > leave the ones accepting the port arg. > >>>> However, the URL class doesn't object and throw a >>>> MalformedURLException, which is something of an >>>> anomaly, caused by the spec allowing a port value of -1 for the 4 arg >>>> constructor, which maps to >>>> the default value for the associated protocol. Using an input port >>>> value of -1 is unnecessary, as the 3 arg constructor >>>> is sufficient to specify a URL with a default port value. >>> >>> Right, it could be argued that the original authors added to much >>> “convenience” here. >>> >>>> It would seem more appropriate to encapsulate the >>>> -1 value as an implementation detail. >>> >>> Right, it is a pity that the ‘-1’ has made it into the spec :-( >>> >>>> A further anomaly arises when a subsequent getPort() method is >>>> invoked, and this returns a value of -1, rather than >>>> the protocol's associated default value. The -1 doesn't seem to >>>> activate until a connection is made using the URL's details. >>> >> I think we can remove almost all references in the javadoc to the >> special -1 port value. > > I don't think we can do this without breaking many things. And it adds > little value. > > > getPort() is one place where it cannot >> removed, and the mention for the constructor with the 5 parameters could >> be remove when we introduce a constructur >> with almost the same signature just leave the port away. But i am not >> sure if intrudicing another constructor is worth the >> -1 removale in the javadoc. >> >> If it is not worth it, the -1 reference can at least be removed in the 2 >> and 3 parameter constructors javadoc. I will prepare this. >> >> What do you think? > > I do not think this is worth pursuing. > > -Chris. > >> -- Sebastian >>> Another unfortunate mistake. >>> >>>> a more generalized description for MalformedURLException could be >>>> used, e.g. >>>> >>>> if the parsed URL fails to comply with the scheme specific syntax of >>>> the associated protocol. >>> >>> Is this suggested wording for the “spec” accepting constructors? I >>> think what we have for the constructors accepting protocol, host, >>> port, etc, is more accurate. >>> >>> -Chris. >>> >>>> regards >>>> Mark >>>> >>>> >>>> >>>> On 10/09/2015 20:32, Sebastian Sickelmann wrote: >>>>> Hi, >>>>> >>>>> first thanks to Chris and David for their helpful input . I looked >>>>> through the existing >>>>> Testcases and found one that is already testing for negative-port >>>>> numbers. >>>>> So i extended the @bug line with "4906983" which I hope is the right >>>>> solution to do it. >>>>> >>>>> I am with Chris, when he says normally you only have numbers between >>>>> 1 and >>>>> 65535 (because many protocols are using tcp). So i changed to >>>>> documentation as >>>>> Chris suggested it. >>>>> >>>>> But ports above this "natural" barrier are valid too. It depends on >>>>> the protocol what >>>>> to do with the port information. So I also extended the testcase to >>>>> check that their are >>>>> valid port numbers also above 65535 and the special -1. >>>>> >>>>> But i asked myself should >>>>> new URL("http://server:-1/path";); >>>>> be realy a valid URL? What do you think? >>>>> >>>>> Special thanks to David who hosted the new webrev: >>>>> http://cr.openjdk.java.net/~dbuck/4906983.1/ >>>>> >>>>> -- Sebastian >>>>> >>>>> >>>>> Am 10.09.2015 um 12:38 schrieb Chris Hegarty: >>>>>> Another minor comment... >>>>>> >>>>>> While what you have suggested is not incorrect, I’m afraid it is >>>>>> giving the wrong impression about the typical acceptable port >>>>>> ranges. A port of Integer.MAX_VALUE is not all that useful, since >>>>>> it typically maps to a TCP port number ( but not always ). Maybe >>>>>> just remove the valid values from @param port, and add something >>>>>> like the following to MalformedURLException: “.., or the port is >>>>>> a negative number other than -1” ? >>>>>> >>>>>> -Chris. >>>>>> >>>>>> On 10 Sep 2015, at 11:13, Chris >>>>>> Hegarty<chris.hega...@oracle.com> wrote: >>>>>> >>>>>>> On 8 Sep 2015, at 21:01, Sebastian >>>>>>> Sickelmann<sebastian.sickelm...@gmx.de> wrote: >>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> Please find my small patch[1] to javadoc in java.net.URL that >>>>>>>> adresses >>>>>>>> JDK-4906983(javadoc-fix)[2]. >>>>>>>> >>>>>>>> I signed the SCA/OCA some time ago. Feel free to check at the OCA >>>>>>>> Signatures-List[3] >>>>>>>> >>>>>>>> thanks to david buck for hosting this patch >>>>>>>> oncr.openjdk.java.net <http://cr.openjdk.java.net>. >>>>>>>> >>>>>>>> -- Sebastian Sickelmann >>>>>>>> >>>>>>>> [1]http://cr.openjdk.java.net/~dbuck/4906983.0/ >>>>>>> Just to confirm this is a spec only change, that documents long >>>>>>> standing existing behaviour, right? >>>>>>> >>>>>>> I think we should add a minimal testcase to cover this. >>>>>>> >>>>>>> Thanks, >>>>>>> -Chris. >>>>>>> >>>>>>>> [2]https://bugs.openjdk.java.net/browse/JDK-4906983 >>>>>>>> >>>>>>>> [3]http://www.oracle.com/technetwork/community/oca-486395.html >>>>> >>>> >>> >> >
