RFR: 8253742: POSIX signal code cleanup [v5]

[David Holmes]

Let me just add a little to what Thomas says ...

On 10/11/2020 6:55 am, Thomas Stuefe wrote:
> On Mon, 9 Nov 2020 15:55:13 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:
> 
>>>> Coleen asked me to remove <signal.h> from the signals_posix.hpp, so those are forward declarations for the signal types we use.
>>>> I thought it was a reasonable request to minimize the number of headers. I saw some efforts in the past to cleanup header files, which is supposed to help with build times, so every little bit helps.
>>>
>>>
>>> The only reason we would care is if signals_posix.hpp were included in
>>> many other handers/files and that should not be the case. This looks
>>> completely bogus to me as we need the types from signal.h in this header
>>> file.
>>
>> Just trying to learn: why do we **need** them? We only included them in the APIs here, but we don't actually **use** them otherwise.

They are external types that we need to use in the declarations of our 
API, so the general rule is that you include the header that defines 
those types (directly or knowingly indirectly). That applies to hotspot 
types and system types in the first instance.

>>
>> And if we need to include `<signal.h>` then don't we also need to include the headers for `outputStream`, `Thread` and `OSThread`? All of these types are used to define the APIs in this header file and are used in the same capacity.

For hotspot, as our headers are somewhat of a mess, we often have to 
simplify things (ie avoid circular dependencies) by eliding includes if 
we can get away with simple class forward declarations. e.g.

class Thread;
class outputStream;

More recently there has been a push to improve build times by detecting 
excessive includes of particular headers, and avoiding those includes by 
using forward declarations if possible; or by moving code to cpp files.

To me this current situation with posix_signal.hpp and signal.h does not 
meet any of the criteria to try and avoid the include.

> You need to include them if you:
> - use the full type, so whoever compiles the header has to know the type size. E.g. if you pass a structure by value.
> - use the type as pointer and do not forward declare it.
> 
> In hotspot, it is standard practice to forward-declare structures from some hotspot utility headers - eg ostream.hpp - to avoid including them. There is no guideline of when to do this, and obviously there is a point at which it is simpler and clearer to just include the header.
> 
> In my personal opinion forward declaration is just a bandaid to work around badly designed headers. Ideally, headers should be small and concise. But hotspot headers are balls of yarn. Pull one in you get a bunch of unrelated others. So people got used to forward declaring some classes instead, things like outputStream. I actually think its a bad practice and in the ideal world we would just include whatever we need.
> 
> Which also means that the benefit of forward-declaring types from system headers is limited. Posix headers are usually well designed, and you are better off just including them. Especially since you need to be careful here: it is not clear what these opaque posix types are actually. Sometimes the standard tells you: "The <signal.h> header shall define the siginfo_t type as a structure.." but sometimes it leaves it open: "sigset_t ... Integer or structure type ...".
> 
>>
>>> What do those typedefs even mean? I would expect a forward
>>> declaration to be of the form:
>>> struct siginfo_t;
>>> but you don't know what type sigset_t (could be integer or struct)
>>> actually is so you can't forward declare it that way.
>>
>> Forward declaration was a new concept to me, so I had to look it up and although it was easy enough to do it for the internal c++ classes, I struggled with the forward declaring of `struct sigset_t`, but I thought I found it.
>>
>> Should I also drop the forward declaration for `outputStream`, `Thread`, `OSThread`?
> 
> For hotspot classes, I would leave the forward declarations in and the headers out. Current standard practice. System headers OTOH I would either include here or, somewhat better, in globalDefinitions_gcc.hpp.

Ha! It is already in globalDefinitions_gcc.hpp so neither the direct 
include nor the forward declarations are actually needed.

Cheers,
David
-----

> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/636
>