[vim/vim] Update style guides and add formatter (PR #15789)
Luca Saccarola
Hi,
I've been working on #15633 but I found myself doing really large refactors. So I thought that it would be better to gradually improve the code and then work on the feature.
The problem
This PR tries to solve two problems:
- We don't have a well defined style/coding style
- We don't have an easy way to make the code look more consistent (aka formatting)
As I was reading the codebase more deeply. I found really different coding styles been used. Sometimes even in the same file.
Examples
Sometimes enums or structs don't return the '{'
// structs.h typedef struct { // ... } spellvars_T; // structs.h typedef enum { // ... } xp_prefix_T;
Most of the time variables are first declared and then later assigned but I found instances were is not the case.
I even found at some point the NASA convention to cast to void unwanted returns values.
// sign.c (void)sign_jump(id, group, buf);
This isn't the end of the world I know. But for a new developer that is willing to work on vim, such as me, is very confusing to know what is the way to do things.
Further more the current style isn't really good to read. Mainly due to:
- Really big functions with lots of moving parts
- Really nested code sometimes with conditional preprocessor stuff
- Big if else statement were the prefixing if or leading else as no bracket (I found myself rereading multiple time to spot that)
- Using a lot of globally scoped function variable even when not needed
- Having really messy typedefs detacched from the struct/enum itself (having typedefs close to the struct you are typedeffing helps when going to definition).
- Redundant branches (I found a lot of ifs that weren't even needed
- Not giving default values to variable (I refactored one and spend a lot of time figuring out why my integer was a big random number, only to find out it hadn't a default value).
Solution
Note
I will myself doing the gradual moving. I love vim and I just want to keep improving it.
Note
The clang-format is based of the 'gnu' style in clang-format and some style changes are loosely inspired by the linux kernel
- I've updated the
develop.txtfile some point I was mentioning above and try to help improve the reliability of the code. - I've added a clang-format with the desired formatting style
Adopting clang-format is going to drop some formatting rule that were followed:
- Reliable alignment of varibles names in structs and functions.
- There is an option for clang format to align variable names but is really clunky and doesn't work well with the preprocessor
- Return types will no longer be indented by one 'shiftwidth'
- I couldn't find and option to do so but I think having a formatter is better that having indented return types.
Q: Why clang-format and not 'insert-formatter-name'?
clang-format is widely used in the C world and probably a C developer as it installed already.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/15789
Commit Summary
File Changes
(3 files)
- A .clang-format (254)
- M runtime/doc/develop.txt (79)
- M src/sign.c (2992)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan commented on this pull request.
In src/sign.c:
> - int sn_cul_hl; // highlight ID for text on current line when 'cursorline' is set - int sn_num_hl; // highlight ID for line number - int sn_priority; // default priority of this sign, -1 means SIGN_DEF_PRIO + sign_T *sn_next; // next sign in list + int sn_typenr; // type number of sign + char_u *sn_name; // name of sign + char_u *sn_icon; // name of pixmap +#ifdef FEAT_SIGN_ICONS + void *sn_image; // icon image +#endif + char_u *sn_text; // text used instead of pixmap + int sn_line_hl; // highlight ID for line + int sn_text_hl; // highlight ID for text + int sn_cul_hl; // highlight ID for text on current line when 'cursorline' is set + int sn_num_hl; // highlight ID for line number + int sn_priority; // default priority of this sign, -1 means SIGN_DEF_PRIO
The Vim coding convention is to align all the variable names in a structure using 'shiftwidth' (and 'softtabstop'). With this new indentation, the names are not aligned.
In src/sign.c:
> sign_group_ref(char_u *groupname)
{
- hash_T hash;
- hashitem_T *hi;
- signgroup_T *group;
+ hash_T hash;
+ hashitem_T *hi;
+ signgroup_T *group;
The Vim coding convention for local variable declarations is to align the variable names (using 'softtabstop').
In src/sign.c:
> } /* * A new sign in group 'groupname' is added. If the group is not present, * create it. Otherwise reference the group. */ - static signgroup_T * +static signgroup_T *
The Vim coding convention is to use one 'shiftwidth' for the function return type.
In src/sign.c:
> - // new group - group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); - if (group == NULL) - return NULL; - STRCPY(group->sg_name, groupname); - group->sg_refcount = 1; - group->sg_next_sign_id = 1; - hash_add_item(&sg_table, hi, group->sg_name, hash); + // new group + group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); + if (group == NULL) + return NULL; + STRCPY(group->sg_name, groupname); + group->sg_refcount = 1; + group->sg_next_sign_id = 1; + hash_add_item(&sg_table, hi, group->sg_name, hash);
The coding convention is to use 'softtabstop' to align the code.
In src/sign.c:
> }
else
{
- // existing group
- group = HI2SG(hi);
- group->sg_refcount++;
+ // existing group
+ group = HI2SG(hi);
+ group->sg_refcount++;
I don't think this PR should be merged. This doesn't follow the existing coding convention (that Bram diligently maintained for 30 years).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
@saccarosium commented on this pull request.
In src/sign.c:
> - // new group - group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); - if (group == NULL) - return NULL; - STRCPY(group->sg_name, groupname); - group->sg_refcount = 1; - group->sg_next_sign_id = 1; - hash_add_item(&sg_table, hi, group->sg_name, hash); + // new group + group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); + if (group == NULL) + return NULL; + STRCPY(group->sg_name, groupname); + group->sg_refcount = 1; + group->sg_next_sign_id = 1; + hash_add_item(&sg_table, hi, group->sg_name, hash);
Yes, I know that but it gives you mixed tabs and spaces witch can be annoying. Also this technically is following that because it uses 4 spaces as it is stated in the modeline at the top of the file.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
> } /* * A new sign in group 'groupname' is added. If the group is not present, * create it. Otherwise reference the group. */ - static signgroup_T * +static signgroup_T *
At the end of the description of the issue I acknowledge this and the main problem is that no formatter support this kind of formatting (at least the one I've tried)
- Return types will no longer be indented by one 'shiftwidth'
- I couldn't find and option to do so but I think having a formatter is better that having indented return types.
In my opinion this is not a blocker for having a formatter because will improve signicantly the consistence of the code
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
> sign_group_ref(char_u *groupname)
{
- hash_T hash;
- hashitem_T *hi;
- signgroup_T *group;
+ hash_T hash;
+ hashitem_T *hi;
+ signgroup_T *group;
Also acknoledge in at the end of the description
- Reliable alignment of varibles names in structs and functions.
- There is an option for clang format to align variable names but is really clunky and doesn't work well with the preprocessor
Again this minor change. A formatter is a far better tool to have that alligned variable names.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
> }
else
{
- // existing group
- group = HI2SG(hi);
- group->sg_refcount++;
+ // existing group
+ group = HI2SG(hi);
+ group->sg_refcount++;
This doesn't follow the existing coding convention (that Bram diligently maintained for 30 years)
The 'existing' coding convention is really lose and it isn't followed everywhere. It is mixed and matched and really hard to follow. A formatter with a set of rules can help with unifying the style
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
K.Takata
@k-takata commented on this pull request.
In src/sign.c:
> - "define",
-# define SIGNCMD_DEFINE 0
- "undefine",
-# define SIGNCMD_UNDEFINE 1
- "list",
-# define SIGNCMD_LIST 2
- "place",
-# define SIGNCMD_PLACE 3
- "unplace",
-# define SIGNCMD_UNPLACE 4
- "jump",
-# define SIGNCMD_JUMP 5
- NULL
-# define SIGNCMD_LAST 6
+static char *cmds[] = { "define",
+#define SIGNCMD_DEFINE 0
A space is needed between # and define.
IndentPPDirectives: AfterHash
PPIndentWidth: 1
In src/sign.c:
> -/* + /*
/* should not be indented here.
In src/sign.c:
> + if (lnum == sign->se_lnum && id == sign->se_id + && sign_in_group(sign, groupname))
Additional indentation is needed in the if-condition:
⬇️ Suggested change- if (lnum == sign->se_lnum && id == sign->se_id - && sign_in_group(sign, groupname)) + if (lnum == sign->se_lnum && id == sign->se_id + && sign_in_group(sign, groupname))
In src/sign.c:
> + switch (cmd_idx)
+ {
+ case SIGNCMD_DEFINE:
+ expand_what = EXP_DEFINE;
+ break;
Additional indentation is needed inside the switch-block:
⬇️ Suggested change- switch (cmd_idx)
- {
- case SIGNCMD_DEFINE:
- expand_what = EXP_DEFINE;
- break;
+ switch (cmd_idx)
+ {
+ case SIGNCMD_DEFINE:
+ expand_what = EXP_DEFINE;
+ break;
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
K.Takata
> - // new group - group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); - if (group == NULL) - return NULL; - STRCPY(group->sg_name, groupname); - group->sg_refcount = 1; - group->sg_next_sign_id = 1; - hash_add_item(&sg_table, hi, group->sg_name, hash); + // new group + group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); + if (group == NULL) + return NULL; + STRCPY(group->sg_name, groupname); + group->sg_refcount = 1; + group->sg_next_sign_id = 1; + hash_add_item(&sg_table, hi, group->sg_name, hash);
witch can be annoying.
It should not be an issue if you use Vim.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
@saccarosium commented on this pull request.
In src/sign.c:
> - // new group - group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); - if (group == NULL) - return NULL; - STRCPY(group->sg_name, groupname); - group->sg_refcount = 1; - group->sg_next_sign_id = 1; - hash_add_item(&sg_table, hi, group->sg_name, hash); + // new group + group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); + if (group == NULL) + return NULL; + STRCPY(group->sg_name, groupname); + group->sg_refcount = 1; + group->sg_next_sign_id = 1; + hash_add_item(&sg_table, hi, group->sg_name, hash);
I would like to make the codebase accessible outside vim. I only use vim, but, I can see an user that maybe wants to contribute but use an editor in witch is more comfortable.
Also this help in general to make the style more consistent across displaying methonds.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
@benknoble commented on this pull request.
In src/sign.c:
> }
else
{
- // existing group
- group = HI2SG(hi);
- group->sg_refcount++;
+ // existing group
+ group = HI2SG(hi);
+ group->sg_refcount++;
In particular, automatic code formatting helps scale the number of possible contributors and maintainers by requiring less effort than Bram surely put in over 30 years of keeping this (bespoke) convention. In other words, I deeply appreciate Bram's efforts, and I don't wish another 30 years of carefully reviewing indents on anyone.
If keeping these conventions are desired, I suggest trying to upstream required changes to clang-format (or, at worst, maintaining a fork). Or perhaps a lesser known formatter exists which supports these options? Or perhaps writing our own is not so hard? (That last is tongue-in-cheek: handling the pre-processor is challenging, although we'd be able to encode knowledge of how Vim uses it into the custom program.)
Finally, I'm not saying this PR is the correct application of the idea to Vim—I am suggesting to seriously consider the idea of automated code formatting for Vim's code base (a de facto standard among open source projects these days). There are many possible directions, and some involve tradeoffs that include adjustments to the historic style of the code. Those should be weighed empathetically with Bram's memory and socially and technically with those who carry on his legacy.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
@saccarosium pushed 4 commits.
- 364fa10 fix preprocessor statement indentation
- 0e43796 fix indentation in case statements
- 0303462 fix trailing comment alignment
- d2f63c0 fix general formatting
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
@saccarosium commented on this pull request.
In src/sign.c:
> + switch (cmd_idx)
+ {
+ case SIGNCMD_DEFINE:
+ expand_what = EXP_DEFINE;
+ break;
This should be resolved
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
> + if (lnum == sign->se_lnum && id == sign->se_id + && sign_in_group(sign, groupname))
I can't find an option to tweak this.
This is the closest thing I could find but doesn't really affect the indentation
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
> -/* + /*
Should be resolved
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
> - "define",
-# define SIGNCMD_DEFINE 0
- "undefine",
-# define SIGNCMD_UNDEFINE 1
- "list",
-# define SIGNCMD_LIST 2
- "place",
-# define SIGNCMD_PLACE 3
- "unplace",
-# define SIGNCMD_UNPLACE 4
- "jump",
-# define SIGNCMD_JUMP 5
- NULL
-# define SIGNCMD_LAST 6
+static char *cmds[] = { "define",
+#define SIGNCMD_DEFINE 0
Done
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Philip H.
The new file .clang-format needs to be on the FileList
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
@saccarosium pushed 1 commit.
- c9123e1 add .clang-format to FileList
You are receiving this because you are subscribed to this thread.
Christian Clason
@clason commented on this pull request.
In src/sign.c:
> - // new group - group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); - if (group == NULL) - return NULL; - STRCPY(group->sg_name, groupname); - group->sg_refcount = 1; - group->sg_next_sign_id = 1; - hash_add_item(&sg_table, hi, group->sg_name, hash); + // new group + group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); + if (group == NULL) + return NULL; + STRCPY(group->sg_name, groupname); + group->sg_refcount = 1; + group->sg_next_sign_id = 1; + hash_add_item(&sg_table, hi, group->sg_name, hash);
It should not be an issue if you use Vim.
It is if you use the builtin commenter ;)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
Thanks. In general I am sympathic to having automatic code formatting. However, it seems those are quite massive changes and this is hard to review.
Perhaps, if we all can agree on a new style, we can start gradually improving the formatting, something similar to what Neovim has been doing?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
if we all can agree on a new style
I don't know usually this kind of discussions leads to nothing most of the time. But I could be wrong about that.
TBH I don't really care for the standard itself as long as it is consistent and automatic.
we can start gradually improving the formatting, something similar to what Neovim has been doing?
I don't know how we can do it more gradually. Do you mean picking a smaller file?
However, it seems those are quite massive changes and this is hard to review.
I see. I would love to make this happen so anything I can do to make this more manageable I will try my best.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
I tend to be sympathetic to those changes and getting rid of tabs is a good change in my opinion (although we lose some alignments). I leave this open for a few days, but will probably merge it, unless other maintainers strongly disagree.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
D. Ben Knoble
Future readers frustrated with git blame may be interested in some options I pointed out in #15829 (comment)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Gary Johnson
> I tend to be sympathetic to those changes and getting rid of tabs is a good
> change in my opinion (although we lose some alignments). I leave this open for
> a few days, but will probably merge it, unless other maintainers strongly
> disagree.
a previous commenter, I would never trade off readability for an
automatic formatter, nor would I use a formatter without checking
what it just formatted for readability and consistency with the
style used in that file.
Regards,
Gary
vim-dev ML
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Luca Saccarola
Hi @chrisbra (sorry for the ping),
there is something stopping this? I'm planning to work on better wayland support (since I desperately need it). I would like to do it "right way", in the formatting and coding style department, in the first go. So it will be less work later on since I think would be a large change.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
K.Takata
@k-takata commented on this pull request.
In src/sign.c:
> - // new group - group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); - if (group == NULL) - return NULL; - STRCPY(group->sg_name, groupname); - group->sg_refcount = 1; - group->sg_next_sign_id = 1; - hash_add_item(&sg_table, hi, group->sg_name, hash); + // new group + group = alloc(offsetof(signgroup_T, sg_name) + STRLEN(groupname) + 1); + if (group == NULL) + return NULL; + STRCPY(group->sg_name, groupname); + group->sg_refcount = 1; + group->sg_next_sign_id = 1; + hash_add_item(&sg_table, hi, group->sg_name, hash);
Changing this makes reviewing harder.
At least, it should be a separate PR.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
dkearns
@dkearns commented on this pull request.
In src/sign.c:
> sign_group_ref(char_u *groupname)
{
- hash_T hash;
- hashitem_T *hi;
- signgroup_T *group;
+ hash_T hash;
+ hashitem_T *hi;
+ signgroup_T *group;
I tried setting AlignConsecutiveDeclarations and it did indeed fail to align the struct members. There's a related issue for this: llvm/llvm-project#109768
I've never used this tool but I can't say it is inspiring much confidence. Have you tried running it across the entire project and building?
struct sign {
sign_T *sn_next; // next sign in list
int sn_typenr; // type number of sign
char_u *sn_name; // name of sign
char_u *sn_icon; // name of pixmap
# ifdef FEAT_SIGN_ICONS
void *sn_image; // icon image
# endif char_u *sn_text; // text used instead of pixmap
int sn_line_hl; // highlight ID for line int sn_text_hl; // highlight ID for text
int sn_cul_hl; // highlight ID for text on current line when 'cursorline' is set
int sn_num_hl; // highlight ID for line number
int sn_priority; // default priority of this sign, -1 means SIGN_DEF_PRIO
};
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
K.Takata
@k-takata commented on this pull request.
In src/sign.c:
> + if (lnum == sign->se_lnum && id == sign->se_id + && sign_in_group(sign, groupname))
I am surprised that clang-format doesn't have this setting.
I think I saw this coding style in the Java coding style many years ago:
if (cond1 && cond2) { statements; }
It is difficult to recognize if cond2 is a part of conditions or statements at a glance.
The additional indentation makes it easier.
Even though we use a different style from Java, I think this setting is a must-have.
if (cond1 && cond2) { statements; }
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Luca Saccarola
@saccarosium commented on this pull request.
In src/sign.c:
> sign_group_ref(char_u *groupname)
{
- hash_T hash;
- hashitem_T *hi;
- signgroup_T *group;
+ hash_T hash;
+ hashitem_T *hi;
+ signgroup_T *group;
Ok for some strange reasons it doesn't compile anymore if I format all the project.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Luca Saccarola
Ok this should be investigate further before doing anything. I will create a follow up issue to discuss further this. I would like to distill a clear and detail vision on what the coding style and formatting should look like.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Luca Saccarola
Closed #15789.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
dundargoc
Ok this should be investigate further before doing anything.
Why? He literally said he'd merge this unless any maintainers strongly disagreed, which they haven't. You will not be making any progress on this by having a drawn out discussion, it is going to get bikeshedded into the ground. I think you should reopen this and go through with it.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Luca Saccarola
You will not be making any progress on this by having a drawn out discussion, it is going to get bikeshedded into the ground.
You are probably right about this but I need to test better the formatter. It is giving me real problems in other files so I need to test other option and play around with setting some more. If I don't test this more thoroughly it would probably cause more headaches.
Also I simply redo this PR in smaller bits to be more digestible hopefully this would make thing go faster.
I will also try to use uncrustify (as you tried in #13318) and see if it is a better fit than clang-format. Currently clang-format is really annoying in the heavy preprocessor driven word we have in the C core.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.