Обсуждение: Regarding the order of the header file includes
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
'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
Вложения
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
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
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
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
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.
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
have you tried to use 'bash' instead of 'sh' to run this script?
Thanks
Richard
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
order of header file includes in non-backend modules.
Thanks
Richard
Вложения
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)
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
Thanks
Richard