linbox-team / fflas-ffpack

FFLAS-FFPACK - Finite Field Linear Algebra Subroutines / Package
http://linbox-team.github.io/fflas-ffpack/
GNU Lesser General Public License v2.1
56 stars 23 forks source link

Tab or spaces: Emacs and Vim modelines are inconsistent #204

Closed cyrilbouvier closed 5 years ago

cyrilbouvier commented 5 years ago

I did not know where to put this issue as it concerns givaro, fflas and linbox.

Regarding the issues of usings tabs or spaces for indentation, the modelines present at the end of most source code files (embedded config for emacs and vim) are inconstistent:

In Emacs, the name of the setting is indent-tabs-mode:

In Vim, the name of the setting is expandtab:

All givaro files with Emacs modelines have indent-tabs-mode: t. Most fflas files with Emacs modelines have indent-tabs-mode: t, but some contain indent-tabs-mode: nil. All linbox files with Emacs modelines have indent-tabs-mode: nil

All givaro files with Vim modelines have noet. All fflas files with Vim modelines have noet. All but one linbox files with Vim modelines have et

Some files have one config for Emacs and the opposite for Vim (two example are benchmarks/benchmark-fsytrf.C in fflas and linbox/ring/polynomial-local-x.h in linbox)

There are similar inconsistencies regarding how many spaces should be used to replace tabs (8 or 4).

I do not know what is the space/tabs policy for the source files and what should be done about these inconsistencies.

jgdumas commented 5 years ago

I suggest to use tabs everywhere.

Breush commented 5 years ago

I'd vote for spaces instead. But I don't really care. Configuring editors so that it keeps the ident style of the files you're modifying would be great. Don't know if that's easy for everybody.

Otherwise, does emacs/vim can understand editorconfig file? (editorconfig.org) Could be a way to ensure global configuration for new developers.

ClementPernet commented 5 years ago

After some discussions, we agreed a while ago to the setting:

While tabs may seem better suited for indentation, I think spaces are much more suited for multiple users/config editing the same file.

Another argument for spaces: https://stackoverflow.blog/2017/06/15/developers-use-spaces-make-money-use-tabs/

jgdumas commented 5 years ago

ok for me (and I will ask for a raise ;-).

bdsaunders commented 5 years ago

Argh, I use tabs all over the place. Can you make it so that vi gives 4 spaces when I press tab (this doesn't cover all uses...)?

It's good to know why we make no money on LinBox.

On Wed, Feb 6, 2019 at 4:24 AM Jean-Guillaume Dumas < notifications@github.com> wrote:

ok for me (and I will ask for a raise ;-).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/linbox-team/fflas-ffpack/issues/204#issuecomment-460954188, or mute the thread https://github.com/notifications/unsubscribe-auth/ADk6I5b46DmLe9L0pZhVXOGW3U5lrjJdks5vKp8xgaJpZM4aFJ_i .

P1K commented 5 years ago

On vim (vi too?): set tabstop=4 set expandtab should work. I don't know if there's a way to put something at the top of a file to override the .vimrc's settings; that'd be handy.

vneiger commented 5 years ago

I don't know if there's a way to put something at the top of a file to override the .vimrc's settings; that'd be handy.

As far as I understand that's the role of the modeline at the end of the file. And the modeline for vim which seems to be in most files in LinBox (didn't check fflas-ffpack) precisely does this choice (tab = 4 spaces, expand tab, etc), this modeline is: // vim:sts=4:sw=4:ts=4:et:sr:cino=>s,f0,{0,g0,(0,\:0,t0,+0,=s

bdsaunders commented 5 years ago

Thanks. I put those settings in my own .vimrc. If someone also makes it so in the settings at the bottom of the files, all to the good.

I hereby switch my beverage from Tab to Diet Tab.

On Wed, Feb 6, 2019 at 12:28 PM Pierre notifications@github.com wrote:

On vim (vi too?): set tabstop=4 set expandtab should work. I don't know if there's a way to put something at the top of a file to override the .vimrc's settings; that'd be handy.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/linbox-team/fflas-ffpack/issues/204#issuecomment-461111885, or mute the thread https://github.com/notifications/unsubscribe-auth/ADk6I1zugm62Wkpzwz1eJU6vLX6rme6Wks5vKxDSgaJpZM4aFJ_i .

P1K commented 5 years ago

I don't know if there's a way to put something at the top of a file to override the .vimrc's settings; that'd be handy.

As far as I understand that's the role of the modeline at the end of the file. And the modeline for vim which seems to be in most files in LinBox (didn't check fflas-ffpack) precisely does this choice (tab = 4 spaces, expand tab, etc), this modeline is: // vim:sts=4:sw=4:ts=4:et:sr:cino=>s,f0,{0,g0,(0,\:0,t0,+0,=s

Indeed, thanks for the tip! Tho I actually had to change my .vimrc for this to be taken into account: set modeline set modelines=5 (or more; depends how far those comments are located) did the trick.

P1K commented 5 years ago

(But the modeline of fflas is quite different: // vim:sts=8:sw=8:ts=8:noet:sr:cino=>s,f0,{0,g0,(0,\:0,t0,+0,=s So tabs are real tabs and 8-space wide :P

vneiger commented 5 years ago

Ouch... I thought there was some discussion on the tabs/spaces and the modelines had been unified by someone (Dan ?) last Winter, and care was taken to put the vim one on the last line... but maybe that was just in LinBox. If everyone agrees on the modelines, it shouldn't take too long to copy them in all files (although not the most exciting task, I agree). I could do it if no one else volunteers.

The complete modelines used in LinBox:

// Local Variables:
// mode: C++
// tab-width: 4
// indent-tabs-mode: nil
// c-basic-offset: 4
// End:
// vim:sts=4:sw=4:ts=4:et:sr:cino=>s,f0,{0,g0,(0,\:0,t0,+0,=s
P1K commented 5 years ago

I can help too.

vneiger commented 5 years ago

Ok, I'll see if it takes longer than expected then I will tell you, thanks :-)

Thinking about it, I suppose that including these new modelines would not really make sense if the files are not reformatted according to these modelines at the same time. This is easily done but means potentially a lot of changes everywhere (I don't know to which extent the current files have a different formatting)... I hope this won't be too difficult to merge. I'll try to do this today.

ClementPernet commented 5 years ago

@vneiger thanks for volunteering. I confirm that not only should themodelines be updated, but also the whole file should be

vneiger commented 5 years ago

Thank you for the suggestion (I will also remove the trailing whitespaces). I must confess that I know nothing either about these modelines (apart from a few parameters such as tabstop etc mentioned by @P1K above), but while using LinBox it just seemed that this modeline used in LinBox seems to work well.

vneiger commented 5 years ago

PS : for the record, in case this can help vim users to respect the style in the future: applying the modeline to the whole file in vim (including untabifying and auto-indenting) amounts to pressing gg=G . Removing the trailing spaces is a bit less straightforward, but can be done by a single keystroke (e.g. F5) after putting the right thing in vim's config file (see e.g. https://vi.stackexchange.com/a/2285 ).

bgrenet commented 5 years ago

For trailing spaces in vim, simple keystroke is :%s/ *$// in command mode.

romainlebreton commented 5 years ago

Instead of

// Local Variables: // mode: C++ // tab-width: 4 // indent-tabs-mode: nil // c-basic-offset: 4 // End:

A one liner for this would best /* -*- mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Don't forget to process also the .inl files please :-)

ClementPernet commented 5 years ago

fixed in #209