Re: [PATCH v2 1/9] riscv/boot: Add a missing header include
Hi Alistair, On Thu, May 14, 2020 at 11:38 PM Alistair Francis wrote: > > On Thu, May 14, 2020 at 8:34 AM Bin Meng wrote: > > > > On Fri, May 8, 2020 at 3:24 AM Alistair Francis > > wrote: > > > > > > Signed-off-by: Alistair Francis > > > --- > > > include/hw/riscv/boot.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h > > > index 474a940ad5..9daa98da08 100644 > > > --- a/include/hw/riscv/boot.h > > > +++ b/include/hw/riscv/boot.h > > > @@ -21,6 +21,7 @@ > > > #define RISCV_BOOT_H > > > > > > #include "exec/cpu-defs.h" > > > +#include "hw/loader.h" > > > > Why is this needed? Currently this does not break build. > > Currently every c file that includes boot.h also includes loader.h > before it. Which is why the build works fine. We should be able to > include just boot.h though so this is a small fixup to allow that. > Please include such in the commit message to help people understand. Reviewed-by: Bin Meng Regards, Bin
Re: [PATCH v2 1/9] riscv/boot: Add a missing header include
On 5/14/20 5:46 PM, Bin Meng wrote: On Thu, May 14, 2020 at 11:38 PM Alistair Francis wrote: On Thu, May 14, 2020 at 8:34 AM Bin Meng wrote: On Fri, May 8, 2020 at 3:24 AM Alistair Francis wrote: Signed-off-by: Alistair Francis --- include/hw/riscv/boot.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h index 474a940ad5..9daa98da08 100644 --- a/include/hw/riscv/boot.h +++ b/include/hw/riscv/boot.h @@ -21,6 +21,7 @@ #define RISCV_BOOT_H #include "exec/cpu-defs.h" +#include "hw/loader.h" Why is this needed? Currently this does not break build. Currently every c file that includes boot.h also includes loader.h before it. Which is why the build works fine. We should be able to include just boot.h though so this is a small fixup to allow that. I wonder if this is a required convention to make the header file self-contained? The only thing that is offending seems to be the symbol_fn_t typedef. Indeed the use of the symbol_fn_t typedef justifies including its declaration :) Reviewed-by: Philippe Mathieu-Daudé Regards, Bin
Re: [PATCH v2 1/9] riscv/boot: Add a missing header include
On Thu, May 14, 2020 at 11:38 PM Alistair Francis wrote: > > On Thu, May 14, 2020 at 8:34 AM Bin Meng wrote: > > > > On Fri, May 8, 2020 at 3:24 AM Alistair Francis > > wrote: > > > > > > Signed-off-by: Alistair Francis > > > --- > > > include/hw/riscv/boot.h | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h > > > index 474a940ad5..9daa98da08 100644 > > > --- a/include/hw/riscv/boot.h > > > +++ b/include/hw/riscv/boot.h > > > @@ -21,6 +21,7 @@ > > > #define RISCV_BOOT_H > > > > > > #include "exec/cpu-defs.h" > > > +#include "hw/loader.h" > > > > Why is this needed? Currently this does not break build. > > Currently every c file that includes boot.h also includes loader.h > before it. Which is why the build works fine. We should be able to > include just boot.h though so this is a small fixup to allow that. > I wonder if this is a required convention to make the header file self-contained? The only thing that is offending seems to be the symbol_fn_t typedef. Regards, Bin
Re: [PATCH v2 1/9] riscv/boot: Add a missing header include
On Thu, May 14, 2020 at 8:34 AM Bin Meng wrote: > > On Fri, May 8, 2020 at 3:24 AM Alistair Francis > wrote: > > > > Signed-off-by: Alistair Francis > > --- > > include/hw/riscv/boot.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h > > index 474a940ad5..9daa98da08 100644 > > --- a/include/hw/riscv/boot.h > > +++ b/include/hw/riscv/boot.h > > @@ -21,6 +21,7 @@ > > #define RISCV_BOOT_H > > > > #include "exec/cpu-defs.h" > > +#include "hw/loader.h" > > Why is this needed? Currently this does not break build. Currently every c file that includes boot.h also includes loader.h before it. Which is why the build works fine. We should be able to include just boot.h though so this is a small fixup to allow that. Alistair > > > > > void riscv_find_and_load_firmware(MachineState *machine, > >const char *default_machine_firmware, > > Regards, > Bin
Re: [PATCH v2 1/9] riscv/boot: Add a missing header include
On Fri, May 8, 2020 at 3:24 AM Alistair Francis wrote: > > Signed-off-by: Alistair Francis > --- > include/hw/riscv/boot.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/hw/riscv/boot.h b/include/hw/riscv/boot.h > index 474a940ad5..9daa98da08 100644 > --- a/include/hw/riscv/boot.h > +++ b/include/hw/riscv/boot.h > @@ -21,6 +21,7 @@ > #define RISCV_BOOT_H > > #include "exec/cpu-defs.h" > +#include "hw/loader.h" Why is this needed? Currently this does not break build. > > void riscv_find_and_load_firmware(MachineState *machine, >const char *default_machine_firmware, Regards, Bin