RFR: 8253742: POSIX signal code cleanup [v5]

[Thomas Stuefe]

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.
> 
> 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.

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.

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

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