Postgres Pro
Downloads
Home   >   mailing lists

Thread: Regarding the order of the header file includes

Regarding the order of the header file includes

From
Richard Guo
Date:
I think we generally follow the rule that we include 'postgres.h' or
'postgres_fe.h' first, followed by system header files, and then
postgres header files ordered in ASCII value.  I noticed that in some C
files we fail to follow this rule strictly.  Attached is a patch to fix
this.

Back in 2019, we performed the same operation in commits 7e735035f2,
dddf4cdc33, and 14aec03502.  It appears that the code has deviated from
that point onwards.

Please note that this patch only addresses the order of header file
includes in backend modules (and might not be thorough).  It is possible
that other modules may have a similar issue, but I have not evaluated
them yet.

Thanks
Richard
Attachment

Re: Regarding the order of the header file includes

From
Bharath Rupireddy
Date:
On Wed, Mar 6, 2024 at 3:02 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
> I think we generally follow the rule that we include 'postgres.h' or
> 'postgres_fe.h' first, followed by system header files, and then
> postgres header files ordered in ASCII value.  I noticed that in some C
> files we fail to follow this rule strictly.  Attached is a patch to fix
> this.
>
> Back in 2019, we performed the same operation in commits 7e735035f2,
> dddf4cdc33, and 14aec03502.  It appears that the code has deviated from
> that point onwards.
>
> Please note that this patch only addresses the order of header file
> includes in backend modules (and might not be thorough).  It is possible
> that other modules may have a similar issue, but I have not evaluated
> them yet.

+1. I'm just curious to know if you've leveraged any tool from
src/tools/pginclude or any script or such.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Regarding the order of the header file includes

From
Richard Guo
Date:

On Wed, Mar 6, 2024 at 6:25 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
On Wed, Mar 6, 2024 at 3:02 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
> I think we generally follow the rule that we include 'postgres.h' or
> 'postgres_fe.h' first, followed by system header files, and then
> postgres header files ordered in ASCII value.  I noticed that in some C
> files we fail to follow this rule strictly.  Attached is a patch to fix
> this.
>
> Back in 2019, we performed the same operation in commits 7e735035f2,
> dddf4cdc33, and 14aec03502.  It appears that the code has deviated from
> that point onwards.
>
> Please note that this patch only addresses the order of header file
> includes in backend modules (and might not be thorough).  It is possible
> that other modules may have a similar issue, but I have not evaluated
> them yet.

+1. I'm just curious to know if you've leveraged any tool from
src/tools/pginclude or any script or such.

Thanks for looking.

While rebasing one of my patches I noticed that the header file includes
in relnode.c are not sorted in order.  So I wrote a naive script to see
if any other C files have the same issue.  The script is:

#!/bin/bash

find . -name "*.c" | while read -r file; do
  headers=$(grep -o '#include "[^>]*"' "$file" |
            grep -v "postgres.h" | grep -v "postgres_fe.h" |
            sed 's/\.h"//g')

  sorted_headers=$(echo "$headers" | sort)

  results=$(diff <(echo "$headers") <(echo "$sorted_headers"))

  if [[ $? != 0 ]]; then
    echo "Headers in '$file' are out of order"
    echo $results
    echo
  fi
done

Thanks
Richard

Re: Regarding the order of the header file includes

From
Bharath Rupireddy
Date:
On Thu, Mar 7, 2024 at 12:39 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
> While rebasing one of my patches I noticed that the header file includes
> in relnode.c are not sorted in order.  So I wrote a naive script to see
> if any other C files have the same issue.  The script is:
>
> #!/bin/bash
>
> find . -name "*.c" | while read -r file; do
>   headers=$(grep -o '#include "[^>]*"' "$file" |
>             grep -v "postgres.h" | grep -v "postgres_fe.h" |
>             sed 's/\.h"//g')
>
>   sorted_headers=$(echo "$headers" | sort)
>
>   results=$(diff <(echo "$headers") <(echo "$sorted_headers"))
>
>   if [[ $? != 0 ]]; then
>     echo "Headers in '$file' are out of order"
>     echo $results
>     echo
>   fi
> done

Cool. Isn't it a better idea to improve this script to auto-order the
header files and land it under src/tools/pginclude/headerssort? It can
then be reusable and be another code beautification weapon one can use
before the code release.

FWIW, I'm getting the syntax error when ran the above shell script:

headerssort.sh: 10: Syntax error: "(" unexpected

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Regarding the order of the header file includes

From
Richard Guo
Date:

On Fri, Mar 8, 2024 at 6:58 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
On Thu, Mar 7, 2024 at 12:39 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
> While rebasing one of my patches I noticed that the header file includes
> in relnode.c are not sorted in order.  So I wrote a naive script to see
> if any other C files have the same issue.  The script is:
>
> #!/bin/bash
>
> find . -name "*.c" | while read -r file; do
>   headers=$(grep -o '#include "[^>]*"' "$file" |
>             grep -v "postgres.h" | grep -v "postgres_fe.h" |
>             sed 's/\.h"//g')
>
>   sorted_headers=$(echo "$headers" | sort)
>
>   results=$(diff <(echo "$headers") <(echo "$sorted_headers"))
>
>   if [[ $? != 0 ]]; then
>     echo "Headers in '$file' are out of order"
>     echo $results
>     echo
>   fi
> done

Cool. Isn't it a better idea to improve this script to auto-order the
header files and land it under src/tools/pginclude/headerssort? It can
then be reusable and be another code beautification weapon one can use
before the code release.

Yeah, perhaps.  However the current script is quite unrefined and would
require a lot of effort to make it a reusable tool.  I will add it to my
to-do list and hopefully one day I can get back to it.  Feel free to
mess around with it if someone is interested.
 
FWIW, I'm getting the syntax error when ran the above shell script:

headerssort.sh: 10: Syntax error: "(" unexpected

I think the error is due to line 10 containing bash-style syntax.  Hmm,
have you tried to use 'bash' instead of 'sh' to run this script?

Thanks
Richard

Re: Regarding the order of the header file includes

From
Richard Guo
Date:

On Wed, Mar 6, 2024 at 5:32 PM Richard Guo <guofenglinux@gmail.com> wrote:
Please note that this patch only addresses the order of header file
includes in backend modules (and might not be thorough).  It is possible
that other modules may have a similar issue, but I have not evaluated
them yet.

Attached is v2, which also includes the 0002 patch that addresses the
order of header file includes in non-backend modules.

Thanks
Richard
Attachment

Re: Regarding the order of the header file includes

From
Peter Eisentraut
Date:
On 12.03.24 12:47, Richard Guo wrote:
> 
> On Wed, Mar 6, 2024 at 5:32 PM Richard Guo <guofenglinux@gmail.com 
> <mailto:guofenglinux@gmail.com>> wrote:
> 
>     Please note that this patch only addresses the order of header file
>     includes in backend modules (and might not be thorough).  It is possible
>     that other modules may have a similar issue, but I have not evaluated
>     them yet.
> 
> 
> Attached is v2, which also includes the 0002 patch that addresses the
> order of header file includes in non-backend modules.

committed (as one patch)

Re: Regarding the order of the header file includes

From
Richard Guo
Date:

On Wed, Mar 13, 2024 at 10:07 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 12.03.24 12:47, Richard Guo wrote:
>
> On Wed, Mar 6, 2024 at 5:32 PM Richard Guo <guofenglinux@gmail.com
> <mailto:guofenglinux@gmail.com>> wrote:
>
>     Please note that this patch only addresses the order of header file
>     includes in backend modules (and might not be thorough).  It is possible
>     that other modules may have a similar issue, but I have not evaluated
>     them yet.
>
>
> Attached is v2, which also includes the 0002 patch that addresses the
> order of header file includes in non-backend modules.

committed (as one patch)

Thanks for pushing!

Thanks
Richard