Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Zack Weinberg-2
On Wed, Nov 7, 2018 at 6:55 AM Florian Weimer <[hidden email]> wrote:

>* Szabolcs Nagy:
>> On 07/11/18 11:36, Gabriel F. T. Gomes wrote:
>>> On Wed, 07 Nov 2018, Florian Weimer wrote:
>>>>
>>>> I wasn't aware that we have a policy to use tabs.  Most of my changes
>>>> involving new files do not use tabs. 8-/
>>>
>>> Oh, maybe we don't.  Now I feel embarrassed that I complained about it so
>>> many times.
>>
>> why is there an awkward mix of space and tab
>> indentation everywhere in the code then?
>
> It's the Emacs default.  I don't know about vim.
>
> Have we ever discussed changing it?

The Emacs default behavior (tab stops are fixed at 8 spaces apart,
indentation width is variable, indent with a mix of tabs and spaces)
plus the GNU code-formatting standard's 2-space indentation width
produces text that combines the worst properties of tab-only and
spaces-only indentation.  I'm also constantly getting it wrong,
especially since every other project I contribute to mandates
spaces-only indentation so that's what my global settings are.

I don't recall ever seeing it come up before, but I would support
either a change to spaces-only, or a change to "tabs for indentation,
spaces for alignment" (see https://www.emacswiki.org/emacs/SmartTabs
for more details and an emacs minor mode; I don't know if equivalent
automation exists for vim).

Spaces-only would be easier to migrate to incrementally.  Smart tabs
are abstractly better in some ways but I think we'd have to
machine-reformat the entire repository to get rid of all the existing
8-space tabs.
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Florian Weimer-5
* Zack Weinberg:

> On Wed, Nov 7, 2018 at 6:55 AM Florian Weimer <[hidden email]> wrote:
>>* Szabolcs Nagy:
>>> On 07/11/18 11:36, Gabriel F. T. Gomes wrote:
>>>> On Wed, 07 Nov 2018, Florian Weimer wrote:
>>>>>
>>>>> I wasn't aware that we have a policy to use tabs.  Most of my changes
>>>>> involving new files do not use tabs. 8-/
>>>>
>>>> Oh, maybe we don't.  Now I feel embarrassed that I complained about it so
>>>> many times.
>>>
>>> why is there an awkward mix of space and tab
>>> indentation everywhere in the code then?
>>
>> It's the Emacs default.  I don't know about vim.
>>
>> Have we ever discussed changing it?
>
> The Emacs default behavior (tab stops are fixed at 8 spaces apart,
> indentation width is variable, indent with a mix of tabs and spaces)
> plus the GNU code-formatting standard's 2-space indentation width
> produces text that combines the worst properties of tab-only and
> spaces-only indentation.  I'm also constantly getting it wrong,
> especially since every other project I contribute to mandates
> spaces-only indentation so that's what my global settings are.

And non-normalized whitespace leads to spurious source code changes,
which clutters patch submissions.

> I don't recall ever seeing it come up before, but I would support
> either a change to spaces-only, or a change to "tabs for indentation,
> spaces for alignment" (see https://www.emacswiki.org/emacs/SmartTabs
> for more details and an emacs minor mode; I don't know if equivalent
> automation exists for vim).

I have a feeling that smart tabs do not work naturally for compound
statements and other lambda-like constructs.  At the very least, it's a
lot of work for the editor to get this right.  At this point, it just
can reindent on display, I suppose.

> Spaces-only would be easier to migrate to incrementally.

Yes, and we could enforce it for new lines in most files (except
makefiles and binary files) with a commit hook.

Before we do that, we need to fix the other commit hook problems,
though.

> Smart tabs are abstractly better in some ways but I think we'd have to
> machine-reformat the entire repository to get rid of all the existing
> 8-space tabs.

I think they will be problematic if not enforced by the compiler because
the rules are so complex that editors will differ, resulting again in
non-normalized whitespace.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Joseph Myers
In reply to this post by Zack Weinberg-2
On Wed, 7 Nov 2018, Zack Weinberg wrote:

> spaces-only indentation.  I'm also constantly getting it wrong,
> especially since every other project I contribute to mandates
> spaces-only indentation so that's what my global settings are.

It should be possible to have a .dir-locals.el to specify the rules for
glibc source files.  Something like

(
 (nil . ((indent-tabs-mode . t)))
)

to default to tabs.  To default to spaces e.g.

(
 (nil . ((indent-tabs-mode . nil)))
 (makefile-mode . ((indent-tabs-mode . t)))
)

(since you need to avoid space indentation in some contexts in makefiles).  
(Probably there are enough different cases in glibc that more complicated
rules would be needed in practice.)  Personally I'd prefer spaces-only
indentation, at least for C files (with appropriate .dir-locals.el); less
sure what we should do for .S files.

On the subject of any global formatting changes: for files that came from
external sources but are no longer merged from such sources, do people
think global reformattings into GNU style would be appropriate?  (Thinking
in particular of the various files based on fdlibm, a few of which have
been reformatted at some point but most of which haven't.)

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

David Newall
On 8/11/18 3:13 am, Joseph Myers wrote:
> On the subject of any global formatting changes: for files that came from
> external sources but are no longer merged from such sources, do people
> think global reformattings into GNU style would be appropriate?
Reformatting makes it difficult to see what was changed using diff and
so, in my opinion, should never be done; and particularly not for petty
reasons like indent.
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Joseph Myers
On Thu, 8 Nov 2018, David Newall wrote:

> Reformatting makes it difficult to see what was changed using diff and so, in

Naturally it should be done in a separate commit from any substative
changes (and such a change could be verified with build-many-glibcs.py not
to change installed stripped shared libraries for any configuration,
except where assertion line numbers are involved).

> my opinion, should never be done; and particularly not for petty reasons like
> indent.

*Not* reformatting makes parts of the code hard to read, and hard to edit
(as you need to keep fixing up indentation manually every line when
editing code not following the normal style), and in practice the
hard-to-read issue gets worse over time as the formatting does not stay
consistent with a style people aren't familiar with maintaining code in.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Paul Eggert
Joseph Myers wrote:
> *Not*  reformatting makes parts of the code hard to read

I'm with Joseph on this one. Although there are obviously tradeoffs here, we
shouldn't unduly contort our code merely because "git diff" is a convenient tool
for comparing versions. Git and diff are our servants, not our masters.
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

David Newall
In reply to this post by Joseph Myers
On 8/11/18 11:07 am, Joseph Myers wrote:
> On Thu, 8 Nov 2018, David Newall wrote:
>> Reformatting makes it difficult to see what was changed using diff and so, in
> Naturally it should be done in a separate commit from any substative changes

I fear you missed my point.  When looking to see what changed between
two versions, bulk formatting changes hide what you need to see.

> *Not* reformatting makes parts of the code hard to read, and hard to edit
> (as you need to keep fixing up indentation manually every line when
> editing code not following the normal style), and in practice the
> hard-to-read issue gets worse over time as the formatting does not stay
> consistent with a style people aren't familiar with maintaining code in.

I totally disagree that tabs vs spaces makes code hard to read. As to
having to adjust your own practices to follow prevailing style, I wish
there was a kinder answer than "bad luck", but there isn't.  I think no
three programmers agree 100% on style issues, and that's just life.  I
doubt there is more than a tiny minority that love the indenting in
glibc, but it is what it is.

I cannot overstate how a terrible idea I think edits are that merely
adjust formatting, nor how hard it makes subsequent code analysis.

I've worked as a programmer for more than 40 years, so I feel I speak
with considerable experience.  Do not do this.

Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Joseph Myers
On Thu, 8 Nov 2018, David Newall wrote:

> I fear you missed my point.  When looking to see what changed between two
> versions, bulk formatting changes hide what you need to see.

So you use "diff -w".  Or diff before and after a reformatting change
separately, or specify a revision with "git blame" to look at history
before that revision.

> I totally disagree that tabs vs spaces makes code hard to read. As to having

I'm thinking more of e.g. missing spaces around operators making code
harder to read, but tabs *do* make diffs harder to read, especially when
quoted (and mixing tab-indented lines and space-indented lines at the same
indentation level, more so).

I think "should we fix formatting in existing files to follow GNU style
better, including for files that originally came from elsewhere (BSD,
fdlibm) but are now maintained in glibc?" is a separate question from "if
we adopt spaces for indentation in future, should we change in bulk
existing files that use tabs?".

Florian noted the possibility of using commit hooks to enforce a
particular style for new / changed lines, but that would only detect
problems at the point where someone tries to push to the central
repository - whereas if you have a uniform style rule for all .c files
(except maybe for a very small whitelist of files that are generated or
come verbatim from elsewhere), and it can be verified automatically, you
can run such verification in the glibc testsuite and so detect issues
sooner.

(In the case of trailing whitespace, which the commit hooks *do* reject,
we *did* fix it in bulk in existing files some time ago.)

> to adjust your own practices to follow prevailing style, I wish there was a

With a consistent style in a project, you can set your editor to default
to the style of that project for files therein.  Once some files, and even
some parts of some files, have different indentation style from others,
that no longer works so well, and there is extra work indenting every line
manually to match the different style in a particular file, or else the
file gradually gets less consistent (and in practice both occur).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

DJ Delorie-2

Joseph Myers <[hidden email]> writes:
> or specify a revision with "git blame" to look at history before that
> revision.

git blame without a revision will blame the formatting changes, since
they'd have touched many lines.  This is probably my biggest reason to
avoid formatting changes - it hides who last touched a specific line,
adding extra work to track down algorithmic changes.

> Florian noted the possibility of using commit hooks to enforce a
> particular style for new / changed lines, but that would only detect

IMHO if we decided to enforce a style, we should (1) bulk-format the
sources through, say, indent, and (2) auto-bulk-format on commit also.
If we're going to rely on some programmatic style enforcement, we should
only do so IF it removes the burden of that enforcement from the user.

If the burden remains on the user to format the style, any programmatic
blockades will be seen as annoyances and stumbling blocks, not aids to
coding.

> With a consistent style in a project, you can set your editor to default

Except many of us contribute to multiple projects with differing styles.
One editor setting cannot solve that problem.
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Jeff Law
On 11/8/18 11:56 AM, DJ Delorie wrote:

>
> Joseph Myers <[hidden email]> writes:
>> or specify a revision with "git blame" to look at history before that
>> revision.
>
> git blame without a revision will blame the formatting changes, since
> they'd have touched many lines.  This is probably my biggest reason to
> avoid formatting changes - it hides who last touched a specific line,
> adding extra work to track down algorithmic changes.
>
>> Florian noted the possibility of using commit hooks to enforce a
>> particular style for new / changed lines, but that would only detect
>
> IMHO if we decided to enforce a style, we should (1) bulk-format the
> sources through, say, indent, and (2) auto-bulk-format on commit also.
> If we're going to rely on some programmatic style enforcement, we should
> only do so IF it removes the burden of that enforcement from the user.
This where I'd like to go for GCC as well.  But I haven't had much
success with convincing folks.

FWIW, I thought glibc already had some hooks to catch formatting nits
(like trailing whitespace).

Jeff
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Joseph Myers
In reply to this post by DJ Delorie-2
On Thu, 8 Nov 2018, DJ Delorie wrote:

> IMHO if we decided to enforce a style, we should (1) bulk-format the
> sources through, say, indent,

There are enough weird things with macros involved that I suspect indent
would make a mess of - formatting fixes other than changing tabs to spaces
would need to be done in smaller, more readily reviewable pieces rather
than globally trusting the results of indent.  (Changing tabs to spaces is
pretty safe if you do a before and after comparison of installed stripped
shared libraries for all build-many-glibcs.py configurations.)

> and (2) auto-bulk-format on commit also.

git doesn't allow the central repository to say how clones should be
configured (so it can't set filter.*.clean), and once a commit has been
created in a clone and an attempt is made to push it, the push can succeed
or not, but the central repository can't choose to make changes to the
commit contents.  So auto-bulk-format on commit isn't reliably possible
(whereas you can reject pushes for bad formatting, and can make the glibc
testsuite have a test that fails for bad formatting so it doesn't get as
far as an attempt to push).

> > With a consistent style in a project, you can set your editor to default
>
> Except many of us contribute to multiple projects with differing styles.
> One editor setting cannot solve that problem.

But it can be set based on e.g. the directory in which editing (and we can
put configuration for common editors in the glibc source tree, such as
.dir-locals.el).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Zack Weinberg-2
In reply to this post by Joseph Myers
On Wed, Nov 7, 2018 at 11:43 AM Joseph Myers <[hidden email]> wrote:
> It should be possible to have a .dir-locals.el to specify the rules for
> glibc source files.  Something like
>
> (
>  (nil . ((indent-tabs-mode . t)))
> )

I already have this but it doesn't seem to work 100% reliably, and
debugging Emacs is not something I want to spend my limited hacking
time on.

zw
Reply | Threaded
Open this post in threaded view
|

Re: Indentation, tabs, and spaces (was re: [PATCH] support: Implement TEST_COMPARE_STRING)

Patrick McGehearty
In reply to this post by David Newall
On 11/7/2018 9:34 PM, David Newall wrote:

> On 8/11/18 11:07 am, Joseph Myers wrote:
>> On Thu, 8 Nov 2018, David Newall wrote:
>>> Reformatting makes it difficult to see what was changed using diff
>>> and so, in
>> Naturally it should be done in a separate commit from any substative
>> changes
>
> I fear you missed my point.  When looking to see what changed between
> two versions, bulk formatting changes hide what you need to see.
>
>> *Not* reformatting makes parts of the code hard to read, and hard to
>> edit
>> (as you need to keep fixing up indentation manually every line when
>> editing code not following the normal style), and in practice the
>> hard-to-read issue gets worse over time as the formatting does not stay
>> consistent with a style people aren't familiar with maintaining code in.
>
> I totally disagree that tabs vs spaces makes code hard to read. As to
> having to adjust your own practices to follow prevailing style, I wish
> there was a kinder answer than "bad luck", but there isn't.  I think
> no three programmers agree 100% on style issues, and that's just
> life.  I doubt there is more than a tiny minority that love the
> indenting in glibc, but it is what it is.
>
> I cannot overstate how a terrible idea I think edits are that merely
> adjust formatting, nor how hard it makes subsequent code analysis.
>
> I've worked as a programmer for more than 40 years, so I feel I speak
> with considerable experience.  Do not do this.
>

In addition to the issues mentioned above,
a major reformatting will considerably increase the workload
of those who backport upstream bug-fix patches to earlier releases
that are still being demanded by and maintained for customers.
Even the change-over to modern calling conventions added significant
work when I did a recent update merge. Handling a merge after a complete
reformatting would be seriously unpleasant to contemplate.

Perhaps some of the difficulties of the current inconsistent formatting
might be reduced by using the diff option to ignore differences in blank
space?  I.e. if the only diff between two versions is tabs vs spaces
git diff -b (or whatever option flag is used) will report no diffs.

Another possibility that might require significant code work for git
would be for git blame to be enhanced to 'ignore' formatting only changes
and report the 'most recent logic change' instead 'most recent change'.
After all, as someone else said, the tools are there to work for us.

I'm not saying those are the best ways to go, just brainstorming
on alternatives.

- patrick