Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ts etc overrides in files #196

Closed
simendsjo opened this issue Feb 27, 2014 · 19 comments
Closed

Fix ts etc overrides in files #196

simendsjo opened this issue Feb 27, 2014 · 19 comments

Comments

@simendsjo
Copy link
Contributor

This has been noted deep inside a previous ticket..

Every file begins with
/* vi:set ts=8 sts=4 sw=4:

This doesn't work well with the style guide. These must be fixed or removed.

@rjw57
Copy link
Contributor

rjw57 commented Feb 27, 2014

Agree. The question is: should the source files then be re-indented at the same time? If we just remove the modeline from each file then as changes are made to them we end up with inconsistent coding styles within one file which is IMHO worse than the wrong coding style used consistently in one file.

Perhaps as people make changes they should be encouraged to fix files as they are modified? Then after a few months when development has calmed down we can do a final modeline purge + re-indent when it's more convenient to give everyone whitespace merge conflicts :).

@simendsjo
Copy link
Contributor Author

Isn't the indentation 2 across all files? And someone said he/she is going to make an uncrustify config for the coding style and run it. This should fix inconsitencies.

@rjw57
Copy link
Contributor

rjw57 commented Feb 27, 2014

If there's a big uncrustify run then that's the point to strip the modelines out IMHO.

@simendsjo
Copy link
Contributor Author

that's = what's?

@rjw57
Copy link
Contributor

rjw57 commented Feb 27, 2014

No, "that's" == "that is". As in "when X happens, that is the correct point at which to do Y."

@simendsjo
Copy link
Contributor Author

So "remove the modelines in the same commit as uncrustify is run". In that case I agree :)

@rjw57
Copy link
Contributor

rjw57 commented Feb 27, 2014

Yup. Or, preferably, the same PR but different commits.

@mahkoh
Copy link
Contributor

mahkoh commented Feb 27, 2014

What replacement do you propose if modelines are going to be removed?

@simendsjo
Copy link
Contributor Author

The best would probably be to just replace them with the current style
guide.
On Feb 27, 2014 10:39 PM, "mahkoh" notifications@github.com wrote:

What replacement do you propose if modelines are going to be removed?


Reply to this email directly or view it on GitHubhttps://github.com//issues/196#issuecomment-36294769
.

@mahkoh
Copy link
Contributor

mahkoh commented Feb 27, 2014

I don't see how the (currently non-existent) style guide comes into play here. If anything, the modeline has to follow the style guide.

@simendsjo
Copy link
Contributor Author

@mahkoh , It's not non-existant. It's Googles C++ styleguide with a few small modifications after a vote. See #104

@mahkoh
Copy link
Contributor

mahkoh commented Feb 27, 2014

Googles C++ styleguide

A C++ style guide that contains many things that are irrelevant or even incorrect for a C project. Before this can be the style guide of neovim it has to be carefully analyzed, the incorrect things have to be removed, and things only relevant to C but not C++ have to be added.

You should not rush changes based on the current state of that guide.

Either way, if the style guide doesn't allow modelines, then the style guide has to be changed or a proper replacement for modelines has to be found. At this point, since the modelines are already in use and the style guide is not, I don't see why the guide shouldn't adapt to the needs of the project.

@tarruda
Copy link
Member

tarruda commented Feb 27, 2014

I never used modelines, so I can't comment on this. For enforcing project-specific code style, I use local vimrc by @MarcWeber.

@simendsjo
Copy link
Contributor Author

The problem is that all the modelines state values that forces us to use
spaces for indentation. And I read the style guide. It's not that bad, but
requires a couple of modifications.
On Feb 27, 2014 11:00 PM, "mahkoh" notifications@github.com wrote:

Googles C++ styleguide

A C++ style guide that contains many things that are irrelevant or even
incorrect for a C project. Before this can be the style guide of neovim it
has to be carefully analyzed, the incorrect things have to be removed, and
things only relevant to C but not C++ have to be added.

You should not rush changes based on the current state of that guide.

Either way, if the style guide doesn't allow modelines, then the style
guide has to be changed or a proper replacement for modelines has to be
found. At this point, since the modelines are already in use and the style
guide is not, I don't see why the guide shouldn't adapt to the needs of the
project.


Reply to this email directly or view it on GitHubhttps://github.com//issues/196#issuecomment-36296981
.

@mahkoh
Copy link
Contributor

mahkoh commented Feb 27, 2014

The problem is that all the modelines state values that forces us to use
spaces for indentation.

That seems to be consistent with the Google style guide. The concrete values should, of course, be adapted to the style guide.

@davidzchen
Copy link
Contributor

@mahkoh While the Google C++ style guide is a C++ style guide, C++ is still a superset of C. We addressed the C-specific details that the Google style guide lacking in #66 and #104, which is why we added the modifications. If there are additional C-specific details that are ambiguous in the style guide, we will add those as well, but as of now, there are no obvious holes in our coding convention. We also have a lint tool, which is included in PR #192, as well as an uncrustify config that will be used for an initial clean-up.

That said, the coding convention specifies 2 spaces for indentation, and the indentation settings, whether we use a modeline or not, should be:

set ts=2 sts=2 sw=2 et

@mahkoh
Copy link
Contributor

mahkoh commented Mar 3, 2014

C++ is still a superset of C

C++ is not a superset of C. The following things come to mind:

  • inline has a completely different meaning in C. The C++ coding standard dedicates a paragraph to inline. This paragraph is misleading and irrelevant in a C project.
  • The C++ coding standard does not talk about casting malloc since casts are mandatory in C++. This is not the case in C and many prominent C projects don't cast void pointers.
  • C has designated initializers.
  • C has VLAs

@steveno
Copy link
Contributor

steveno commented Mar 10, 2014

@mahkoh I don't want to argue with you, but the man says C is a super-set of C++.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 10, 2014

He says that C++ is a super-set of ANSI C which means C89. (And even that is not quite true as he explains.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants