Horton

Thread

Commits

  1. Make the order of the header file includes consistent

  1. Regarding the order of the header file includes

    Richard Guo <guofenglinux@gmail.com> — 2024-03-06T09:32:31Z

    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

  2. Re: Regarding the order of the header file includes

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-03-06T10:25:29Z

    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

  3. Re: Regarding the order of the header file includes

    Richard Guo <guofenglinux@gmail.com> — 2024-03-07T07:09:35Z

    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

  4. Re: Regarding the order of the header file includes

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-03-08T10:58:25Z

    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

  5. Re: Regarding the order of the header file includes

    Richard Guo <guofenglinux@gmail.com> — 2024-03-12T11:39:16Z

    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

  6. Re: Regarding the order of the header file includes

    Richard Guo <guofenglinux@gmail.com> — 2024-03-12T11:47:01Z

    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

  7. Re: Regarding the order of the header file includes

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-13T14:07:55Z

    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)

  8. Re: Regarding the order of the header file includes

    Richard Guo <guofenglinux@gmail.com> — 2024-03-18T00:39:25Z

    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