gcc -ftabstop option

Started by Peter Eisentrautover 3 years ago5 messages
#1Peter Eisentraut
peter.eisentraut@enterprisedb.com
1 attachment(s)

I suggest that we add the gcc (also clang) option -ftabstop=4.

This has two effects: First, it produces more accurate column numbers
in compiler errors and correctly indents the code excerpts that the
compiler shows with those. Second, it enables the compiler's detection
of confusingly indented code to work more correctly. (But the latter is
only a potential problem for code that does not use tabs for
indentation, so it would be mostly a help during development with sloppy
editor setups.)

Attached is a patch to discover the option in configure.

One bit of trickery not addressed yet is that we might want to strip out
the option and not expose it through PGXS, since we don't know what
whitespacing rules external code uses.

Thoughts?

Attachments:

0001-Add-compiler-option-ftabstop-4-if-available.patchtext/plain; charset=UTF-8; name=0001-Add-compiler-option-ftabstop-4-if-available.patchDownload
From 2095d1616b2ba8e39daa6aac8004936b8a229354 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 21 Jun 2022 12:40:59 +0200
Subject: [PATCH] Add compiler option -ftabstop=4 if available

---
 configure    | 43 +++++++++++++++++++++++++++++++++++++++++++
 configure.ac |  5 +++++
 2 files changed, 48 insertions(+)

diff --git a/configure b/configure
index 7dec6b7bf9..b7420cceea 100755
--- a/configure
+++ b/configure
@@ -6303,6 +6303,49 @@ if test x"$pgac_cv_prog_CC_cflags__ftree_vectorize" = x"yes"; then
 fi
 
 
+  NOT_THE_CFLAGS=""
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -ftabstop=4, for NOT_THE_CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -ftabstop=4, for NOT_THE_CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__ftabstop_4+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${NOT_THE_CFLAGS} -ftabstop=4"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__ftabstop_4=yes
+else
+  pgac_cv_prog_CC_cflags__ftabstop_4=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__ftabstop_4" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__ftabstop_4" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__ftabstop_4" = x"yes"; then
+  NOT_THE_CFLAGS="${NOT_THE_CFLAGS} -ftabstop=4"
+fi
+
+
+  if test -n "$NOT_THE_CFLAGS"; then
+    CPPFLAGS="$CPPFLAGS -ftabstop=4"
+  fi
   #
   # The following tests want to suppress various unhelpful warnings by adding
   # -Wno-foo switches.  But gcc won't complain about unrecognized -Wno-foo
diff --git a/configure.ac b/configure.ac
index d093fb88dd..b1d7ac9fc2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -525,6 +525,11 @@ if test "$GCC" = yes -a "$ICC" = no; then
   PGAC_PROG_CC_VAR_OPT(CFLAGS_UNROLL_LOOPS, [-funroll-loops])
   # Optimization flags for specific files that benefit from vectorization
   PGAC_PROG_CC_VAR_OPT(CFLAGS_VECTORIZE, [-ftree-vectorize])
+  NOT_THE_CFLAGS=""
+  PGAC_PROG_CC_VAR_OPT(NOT_THE_CFLAGS, [-ftabstop=4])
+  if test -n "$NOT_THE_CFLAGS"; then
+    CPPFLAGS="$CPPFLAGS -ftabstop=4"
+  fi
   #
   # The following tests want to suppress various unhelpful warnings by adding
   # -Wno-foo switches.  But gcc won't complain about unrecognized -Wno-foo
-- 
2.36.1

#2Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Peter Eisentraut (#1)
Re: gcc -ftabstop option

At Tue, 21 Jun 2022 12:49:24 +0200, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in

I suggest that we add the gcc (also clang) option -ftabstop=4.

This has two effects: First, it produces more accurate column numbers
in compiler errors and correctly indents the code excerpts that the
compiler shows with those. Second, it enables the compiler's
detection of confusingly indented code to work more correctly. (But
the latter is only a potential problem for code that does not use tabs
for indentation, so it would be mostly a help during development with
sloppy editor setups.)

Attached is a patch to discover the option in configure.

One bit of trickery not addressed yet is that we might want to strip
out the option and not expose it through PGXS, since we don't know
what whitespacing rules external code uses.

Thoughts?

There's no strong reason for everyone to accept that change, but I
myself don't mind whether the option is applied or not.

There was a related discussion.

/messages/by-id/BDE54C55-438C-48E9-B2A3-08EB3A0CBB9F@yesql.se

The -ftabstop option is (to a large extent, not entirely) to warn when tabs and
spaces are mixed creating misleading indentation that the author didn't even
notice due to tabwidth settings? ISTM we are better of getting these warnings
than suppressing them.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Kyotaro Horiguchi (#2)
Re: gcc -ftabstop option

At Tue, 21 Jun 2022 12:49:24 +0200, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in

One bit of trickery not addressed yet is that we might want to strip
out the option and not expose it through PGXS, since we don't know
what whitespacing rules external code uses.

This part seems like a bigger problem than the option is worth.
I agree that we can't assume third-party code prefers 4-space tabs.

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:

There was a related discussion.
/messages/by-id/BDE54C55-438C-48E9-B2A3-08EB3A0CBB9F@yesql.se

The -ftabstop option is (to a large extent, not entirely) to warn when tabs and
spaces are mixed creating misleading indentation that the author didn't even
notice due to tabwidth settings? ISTM we are better of getting these warnings
than suppressing them.

Isn't that kind of redundant given that (a) we have git whitespace
warnings about this and (b) pgindent will take care of any such
problems in the end?

I'll grant the point about compiler warnings possibly not lining up
precisely. But that's yet to bother me personally, so maybe I'm
underestimating its value.

regards, tom lane

#4Daniel Gustafsson
daniel@yesql.se
In reply to: Peter Eisentraut (#1)
Re: gcc -ftabstop option

On 21 Jun 2022, at 12:49, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

Second, it enables the compiler's detection of confusingly indented code to work more correctly.

Wouldn't we also need to add -Wmisleading-indentation for this to trigger any
warnings? Adding -ftabstop only allows the compiler to be able to properly
detect it.

--
Daniel Gustafsson https://vmware.com/

#5Peter Eisentraut
peter.eisentraut@enterprisedb.com
In reply to: Daniel Gustafsson (#4)
Re: gcc -ftabstop option

On 22.06.22 21:48, Daniel Gustafsson wrote:

On 21 Jun 2022, at 12:49, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:

Second, it enables the compiler's detection of confusingly indented code to work more correctly.

Wouldn't we also need to add -Wmisleading-indentation for this to trigger any
warnings?

That is included in -Wall.