RFR: JDK-8293711: Factor out size parsing functions from arguments.cpp [v2]

[Thomas Stuefe]

On Mon, 26 Sep 2022 07:17:29 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>>> > > is the new version acceptable?
>>> > 
>>> > 
>>> > I would have preferred if the parsing code were not placed in a .hpp file, and would have placed it in a .inline.hpp file, to comply with our guidelines. Other than that, I'm OK with this patch.
>>> 
>>> Almost missed your comment :-)
>>> 
>>> I'll split it up as you suggested, then push.
>> 
>> Wait, since the whole thing is all parsing, would that not mean we have just a parseInteger.inline.hpp, without a parseInteger.hpp? Since I'm unsure what would be left to put into parseInteger.hpp. And a parseInteger.inline.hpp without a parseInteger.hpp makes not much sense, or?
>> 
>> We have some other one-trick-pony-headers that are not .inline and contain their whole functionality: powerOfTwo.hpp, population_count.hpp (probably should rename that), pair.hpp...
>
>> > > > is the new version acceptable?
>> > > 
>> > > 
>> > > I would have preferred if the parsing code were not placed in a .hpp file, and would have placed it in a .inline.hpp file, to comply with our guidelines. Other than that, I'm OK with this patch.
>> > 
>> > 
>> > Almost missed your comment :-)
>> > I'll split it up as you suggested, then push.
>> 
>> Wait, since the whole thing is all parsing, would that not mean we have just a parseInteger.inline.hpp, without a parseInteger.hpp? Since I'm unsure what would be left to put into parseInteger.hpp. And a parseInteger.inline.hpp without a parseInteger.hpp makes not much sense, or?
>> 
>> We have some other one-trick-pony-headers that are not .inline and contain their whole functionality: powerOfTwo.hpp, population_count.hpp (probably should rename that), pair.hpp...
> 
> Right. There's a pragmatic side to the rules w.r.t .hpp vs .inline.hpp files. If the file doesn't have dependencies to "non-trivial" header files, then we typically let it slide and put the code in the .hpp, as you have seen in the files you list. Another notable example is atomic.inline.hpp, which was renamed to atomic.hpp. I think we did that, since were too many header files that used Atomic, and it would have been a lot of work to clean that up.
> 
> The questions for me now are:
> 1) Is this header small enough that it doesn't pull in large amounts of other headers?
> 2) Is it likely to be changed in the future to include "non-trivial" headers?
> 3) Is it likely to be used in other .hpp files?
> 
> I think this it is small enough (1), will only be used in .cpp/.inline.hpp files (3), but I'm unsure about (2).
> 
> I'll leave it to your judgement to decide if this is fine to leave in the .hpp file.

Hi @stefank,

sorry, I was gone for vacation and did not want to rush this before leaving.

> > > > > is the new version acceptable?
> > > > 
> > > > 
> > > > I would have preferred if the parsing code were not placed in a .hpp file, and would have placed it in a .inline.hpp file, to comply with our guidelines. Other than that, I'm OK with this patch.
> > > 
> > > 
> > > Almost missed your comment :-)
> > > I'll split it up as you suggested, then push.
> > 
> > 
> > Wait, since the whole thing is all parsing, would that not mean we have just a parseInteger.inline.hpp, without a parseInteger.hpp? Since I'm unsure what would be left to put into parseInteger.hpp. And a parseInteger.inline.hpp without a parseInteger.hpp makes not much sense, or?
> > We have some other one-trick-pony-headers that are not .inline and contain their whole functionality: powerOfTwo.hpp, population_count.hpp (probably should rename that), pair.hpp...
> 
> Right. There's a pragmatic side to the rules w.r.t .hpp vs .inline.hpp files. If the file doesn't have dependencies to "non-trivial" header files, then we typically let it slide and put the code in the .hpp, as you have seen in the files you list. Another notable example is atomic.inline.hpp, which was renamed to atomic.hpp. I think we did that, since were too many header files that used Atomic, and it would have been a lot of work to clean that up.
> 
> The questions for me now are:
> 
> 1. Is this header small enough that it doesn't pull in large amounts of other headers?
> 2. Is it likely to be changed in the future to include "non-trivial" headers?
> 3. Is it likely to be used in other .hpp files?
> 
> I think this it is small enough (1), will only be used in .cpp/.inline.hpp files (3), but I'm unsure about (2).
> 
> I'll leave it to your judgement to decide if this is fine to leave in the .hpp file.

I don't think this file will see much changes in the future. So I think we can leave the implementations in the hpp file. Thank you!

-------------

PR: https://git.openjdk.org/jdk/pull/10252