Add .clang-format #8113

vstumpf · 2024-01-19T08:10:04Z

Addressed Issue(s): N/A
Server Mode: Both
Description of Pull Request:

This PR has 3 things, the clang format file, the application of it on src/common, and a github action.

0). Why a formatter?

It's a quick way to improve the quality of our code. If we enforce one style, it becomes easier to review code: no more huge list of comments about changing whitespace or a newline. By removing this noise, important comments are not lost.

1). The .clang-format file

This configuration requires at least clang-format 9. MSVS 2019 installs clang-format 10 and MSVS 2022 installs 16.0.5.
To check this, find your installation of visual studio and run clang-format.exe --version, like so:

PS C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\Llvm\bin> .\clang-format.exe --version
clang-format version 16.0.5

I'm not sure what version MSVS 2017 currently has. If an active developer is still using 2017, then we can rethink the IndentPPDirectives option, as the BeforeHash argument is the only thing that requires version 9, I think everything else should be supported by 4.

Formatting is only required for people who wish to contribute back to the project, and I think the ones who do are using a newer version of VS.

2) Applying this to src/common

I went ahead and applied this to src/common so we can see the effects of the current configuration. If you like it, great! Let me know. If not, then please put a comment so we can work it out. There's only a couple of options that I'm deadset on, but I'll let you start the discussion c:
Once we can agree on the config, we can apply it to the rest of the directories.

3) The github action.

In the past, people have brought up the idea of a github action that formats the code and makes a commit with the changes. I don't really like this because:

If someone makes a change to their PR without formatting it beforehand, they'll have to git pull their changes, or when they go to git push the remote will reject the changes. Teaching someone how to fix a conflict is not fun, believe me.
Visual Studio (2022 at least, not sure about others) auto-formats the code when you type/save. With the built-in support for clang-format, developers using MSVS won't have to worry about formatting.
If you want to manually run the formatter, Ctrl+K Ctrl+D runs formatting on the whole file, Ctrl+K Ctrl+F runs it on the current selection.
Visual Studio Code users can use an extension like xaver.clang-format to run clang-format as their default formatter.

Instead, we can just have an action that checks to see if the code is formatted; if it isn't, it will print out the changes required to fix the issue. The developer can either use the output of that to fix the formatting, or start using clang-format.

src/common/core.cpp

Lemongrass3110

Some initial comments/findings.

Basically totally in favor of this idea, we just have to think carefully when to merge this. I would like to have some pending PRs merged before applying this, as we otherwise give us a hard time.

src/common/database.hpp

src/common/core.hpp

src/common/cli.cpp

Lemongrass3110 · 2024-01-22T07:50:38Z

src/common/conf.cpp

 config_init(config);
 if (!config_read_file(config, config_filename)) {
- ShowError("%s:%d - %s\n", config_error_file(config),
- config_error_line(config), config_error_text(config));
+ ShowError("%s:%d - %s\n",


ColumnLimit: 0

Do we really want no column limit? Lines get hard to understand at 140+ characters

I feel like these totally depend on the output. Sometimes they can be one line but sometimes like @vstumpf said, it can drag on too far horizontally. The big kicker is to see what this would look like in char/login with all the SQL stuff.

Usually the only long lines are output formats, like this. Where it is not really problematic.
I do agree that you should not put that much logic into a single line, but this should already be detected during review progress. I tend to comment negative on overloaded short-ifs.

As for the SQL queries I totally agree with that as well, but it should be possible to break the strings into multiple lines, even with that configuration and that would make sense, if we see that clang format is breaking it somewhere.

But for simple format parameter lists I would tend to say that nowdays programmer's screens and editors can display more than 100 characters.

Personally I use a split editor, so it's pretty easy to hit the edge of the screen with no limit

Lemongrass3110 · 2024-01-22T09:07:26Z

src/common/des.cpp

- 0x80, 0x40, 0x20, 0x10, 0x08, 0x04, 0x02, 0x01
-};
-
+static const uint8_t mask[8] = {0x80, 0x40, 0x20, 0x10, 0x08, 0x04, 0x02, 0x01};


BreakArrays: true
Or is there another option for how many elements? Right now it seems to use the maximum line length.

There's no option for number of elements. And BreakArrays is only for formatting JSON, not cpp files :c
For cases where we want to ignore the clang format, we can always just // clang-format off and // clang-format on

Sad but true... llvm/llvm-project#61560
Guess we really have to turn clang format off and back on in the relevant places.

Lemongrass3110 · 2024-01-22T09:12:10Z

src/common/socket.hpp

- do { \
- if(session[fd]->rdata_size == session[fd]->rdata_pos){ \
+#define RFIFOREST(fd) (session[fd]->flag.eof ? 0 : session[fd]->rdata_size - session[fd]->rdata_pos)
+#define RFIFOFLUSH(fd) \


No idea which format option is applied here, but this should be fixed.

This is AlignEscapedNewlines, honestly I like the right align, it makes it obvious how far the macro goes.

To me it makes it harder to review here on github, especially because it uses a mix of tabs and spaces.
If you see a \ at the end of the line you know that it still continues. Which is fair and easy I guess.

.clang-format

vstumpf · 2024-01-23T06:11:45Z

pending PRs merged before applying this

It makes sense, but I'm scared this will never get merged because there will always be "another PR". If we can have a list of PRs or a timeline, it'd be nice

.clang-format

Lemongrass3110 · 2024-01-23T20:08:06Z

pending PRs merged before applying this

It makes sense, but I'm scared this will never get merged because there will always be "another PR". If we can have a list of PRs or a timeline, it'd be nice

Yeah that is why I said some. We would have to work that out I guess.
One of the crucial ones is that monster of a PR #7024.
It was hard enough getting this one up to date.

Also please stop force pushing. This only makes it harder to see, which comments you really resolved and is not helpful for reviewing your PR.

vstumpf · 2024-01-25T07:06:33Z

I was force pushing to reapply the format changes, i just find it more consistent to reapply all the changes, but I'll stop

Fixed macro newline alignment

…g-formatter # Conflicts: # src/common/core.cpp # src/common/mmo.hpp # src/common/packets.hpp # src/common/showmsg.cpp # src/common/socket.cpp # src/common/strlib.cpp # src/common/strlib.hpp # src/common/utilities.cpp

src/common/mapindex.cpp

src/common/core.cpp

src/common/des.cpp

- for( i = 0; i*8 < size; i += 8 )
- des_decrypt_block(p);
+ for( i = 0; i * 8 < size; i += 8 ) {
+ des_decrypt_block( p );


src/common/grfio.cpp

- for( i = 0; i < 20 && i < nblocks; ++i )
- des_decrypt_block(&p[i]);
+ for( i = 0; i < 20 && i < nblocks; ++i ) {
+ des_decrypt_block( &p[i] );


src/common/strlib.cpp

These are only supported in version 15 and above

…g-formatter # Conflicts: # src/common/cbasetypes.hpp # src/common/core.cpp # src/common/grfio.cpp # src/common/malloc.cpp # src/common/nullpo.cpp # src/common/sql.cpp # src/common/sql.hpp # src/common/timer.cpp

src/common/strlib.cpp

- {
- ShowError("sv_readdb: Too many columns in line %d of \"%s\" (found %d, maximum is %d).\n", lines, path, columns, maxcols );
+ if(columns > maxcols) {
+ ShowError("sv_readdb: Too many columns in line %d of \"%s\" (found %d, maximum is %d).\n", lines, path, columns, maxcols);


src/common/strlib.cpp

- {
- ShowError("sv_readdb: Too many columns in line %d of \"%s\" (found %d, maximum is %d).\n", lines, path, columns, maxcols );
+ if(columns > maxcols) {
+ ShowError("sv_readdb: Too many columns in line %d of \"%s\" (found %d, maximum is %d).\n", lines, path, columns, maxcols);


src/common/core.cpp

src/common/mapindex.cpp

src/common/core.cpp


- if( fgets(line, sizeof(line), fp) && sscanf(line, "%40s", rev) )
+ if (fgets(line, sizeof(line), fp) && sscanf(line, "%40s", rev)) {


vstumpf force-pushed the clang-formatter branch from 08a2b1a to 5dc241d Compare January 19, 2024 08:10

Add .clang-format file

ccfd38f

vstumpf force-pushed the clang-formatter branch from 5dc241d to e728c9c Compare January 19, 2024 08:27

github-advanced-security bot found potential problems Jan 19, 2024

View reviewed changes

src/common/core.cpp Fixed Show fixed Hide fixed

Lemongrass3110 reviewed Jan 22, 2024

View reviewed changes

vstumpf added 3 commits January 23, 2024 05:43

Prepare repo for formatting

6daf351

Add format checker action

fb680eb

Apply formatting to common

22dc728

vstumpf force-pushed the clang-formatter branch 2 times, most recently from a8b56a4 to 22dc728 Compare January 23, 2024 06:07

Lemongrass3110 reviewed Jan 23, 2024

View reviewed changes

.clang-format Outdated Show resolved Hide resolved

vstumpf and others added 16 commits January 25, 2024 07:19

Move clang format

356d830

Applied formatting without changes

28c684e

Increased ColumnLimit to 200 characters

cb56605

Added 3rdparty regex to IncludeCategories

1623c51

Added AllowShortEnumsOnASingleLine:false

35f2b21

Added documentation for SpacesBeforeTrailingComments

792eb24

Added AlignEscapedNewlines: DontAlign

de45df2

Fixed macro newline alignment

Added InsertBraces: true

71f9fda

Added AfterControlStatements: false

3562da1

Added AfterForeachMacros: false

5f6a40e

Added AfterFunctionDeclarationName: false

4864c88

Added AfterFunctionDefinitionName: false

6810aea

Added AfterIfMacros: false

f347dc7

Added AfterOverloadedOperator: false

5654947

Added AfterRequiresInClause: false

62bcef4

Added AfterRequiresInExpression: false

b3aa8d2

Lemongrass3110 added 4 commits April 6, 2024 01:56

Added BeforeNonEmptyParentheses: false

70a3d24

Added SpacesInParens: Custom

2ef7b01

Added version output to CI script

e812f53

github-advanced-security bot found potential problems Apr 6, 2024

View reviewed changes

Lemongrass3110 added 5 commits April 10, 2024 21:11

Removed SpacesInParens

15f18f7

Disabled unsupported options

5ea9b51

These are only supported in version 15 and above

Merge branch 'master' of https://github.com/rathena/rathena into clan…

c8c75ba

…g-formatter # Conflicts: # src/common/cbasetypes.hpp # src/common/core.cpp # src/common/grfio.cpp # src/common/malloc.cpp # src/common/nullpo.cpp # src/common/sql.cpp # src/common/sql.hpp # src/common/timer.cpp

Try to update clang-format in CI

72d9803

SUDO!

5ac7c8e

github-advanced-security bot found potential problems Apr 10, 2024

View reviewed changes

vstumpf added 2 commits April 21, 2024 00:02

Add spaces after control statements

756ba00

Merge branch 'master' into clang-formatter

466ae82

github-advanced-security bot found potential problems Apr 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add .clang-format #8113

Add .clang-format #8113

vstumpf commented Jan 19, 2024 •

edited

Lemongrass3110 left a comment

Lemongrass3110 Jan 22, 2024

vstumpf Jan 23, 2024

aleos89 Jan 23, 2024

Lemongrass3110 Jan 23, 2024

vstumpf Jan 25, 2024

Lemongrass3110 Jan 22, 2024

vstumpf Jan 23, 2024

Lemongrass3110 Jan 23, 2024

Lemongrass3110 Jan 22, 2024

vstumpf Jan 23, 2024

Lemongrass3110 Jan 23, 2024

vstumpf commented Jan 23, 2024

Lemongrass3110 commented Jan 23, 2024

vstumpf commented Jan 25, 2024


		if( fgets(line, sizeof(line), fp) && sscanf(line, "%40s", rev) )
		if (fgets(line, sizeof(line), fp) && sscanf(line, "%40s", rev)) {

Add .clang-format #8113

Are you sure you want to change the base?

Add .clang-format #8113

Conversation

vstumpf commented Jan 19, 2024 • edited

0). Why a formatter?

1). The .clang-format file

2) Applying this to src/common

3) The github action.

Lemongrass3110 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstumpf commented Jan 23, 2024

Lemongrass3110 commented Jan 23, 2024

vstumpf commented Jan 25, 2024

vstumpf commented Jan 19, 2024 •

edited