Code formatting by louib · Pull Request #1191 · keepassxreboot/keepassxc

louib · 2017-11-17T18:04:23Z

@droidmonkey @adolfogc @TheZ3ro what would you think about this?

How has this been tested?

cd build
make format

TheZ3ro · 2017-11-17T18:33:59Z

This will need all the code correctly formatted before merging

Anyway seems nice to me!

louib · 2017-11-17T19:05:34Z

@TheZ3ro we could do the formatting of all the code just before the next major, to avoid conflicts when backporting / upporting changes.

adolfogc · 2017-11-18T04:57:35Z

.travis.yml


 script:
  - cmake -DCMAKE_BUILD_TYPE=${CONFIG} -DWITH_GUI_TESTS=ON -DWITH_ASAN=ON -DWITH_XC_HTTP=ON -DWITH_XC_AUTOTYPE=ON -DWITH_XC_YUBIKEY=ON $CMAKE_ARGS ..
+  - make format; git diff --exit-code


adolfogc · 2017-11-18T04:58:15Z

@louib Nice work!

adolfogc · 2017-11-18T14:55:07Z

we could do the formatting of all the code just before the next major, to avoid conflicts when backporting / upporting changes.

I would like to propose the use of clang-tidy when making this one-time refactoring, as it could help fix some cases of missing readability braces around statements. Other checks could also be used, like the modernize-* ones.

adolfogc · 2017-11-18T15:25:56Z

.github/CONTRIBUTING.md


-This project follows the [Qt Coding Style](https://wiki.qt.io/Qt_Coding_Style). All submissions are expected to follow this style.
+The coding style of the project is enforced using llvm's `clang-format` formatting tool. A thorough description
+of the coding style can be found in the `.clang-format` file, but the main conventions are presented here.


Would it be useful to mention here the use of //clang-format off and //clang-format on for having ClangFormat ignore certain sections of the code? To illustrate the possible use-cases, an example could also be provided. I'm thinking on something like:

When deemed necessary, you can use //clang-format off and //clang-format on to have ClangFormat ignore certain parts of the code. For instance, from project's code-base:

// clang-format off int binary = ((hmac[offset] & 0x7f) << 24) | ((hmac[offset + 1] & 0xff) << 16) | ((hmac[offset + 2] & 0xff) << 8) | (hmac[offset + 3] & 0xff); // clang-format on

In the example above, the manual formatting resulted in more readable code than the ClangFormat changes, hence the need for using the special comment lines.

adolfogc · 2017-11-18T23:02:53Z

@louib After trying this branch, I have another observation:

The steps I followed:

git clone https://github.com/louib/keepassxc.git
cd keepassxc
git checkout code_formatting
mkdir build && cd build
cmake ..
make format

The error I got:

Configuration file(s) do(es) not support Objective-C: /Users/adolfogc/Downloads/keepassxc/.clang-format
make[3]: *** [CMakeFiles/format] Error 1
make[2]: *** [CMakeFiles/format.dir/all] Error 2
make[1]: *** [CMakeFiles/format.dir/rule] Error 2
make: *** [format] Error 2

❗️ Please note ClangFormat did affect the project files as expected.

Possible solution:

See my other comments in the code.

adolfogc · 2017-11-18T23:03:33Z

cmake/CLangFormat.cmake

+#  You should have received a copy of the GNU General Public License
+#  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set(THIRD_PARTY_DIRS


Include autotype/mac here.

adolfogc · 2017-11-18T23:04:01Z

cmake/CLangFormat.cmake

+    streams/QtIOCompressor
+)
+
+set(THIRD_PARTY_FILES


Include core/ScreenLockListenerMac.h and core/ScreenLockListenerMac.cpp here.

Would be nice to have all the third party files centralised in one directory...

I'm not sure those two are third party code, they need to be excluded because they contain Objective-C code.

They are not third-party.

Maybe we can exclude them in another group? 🤔

I just renamed the lists so that we can exclude both third-party and obj-c files.

TheZ3ro · 2017-11-19T17:04:14Z

I like this solution @louib

louib · 2017-11-19T17:12:36Z

@adolfogc I tried setting up clang-tidy but wasn't able to come up with a complete solution with the limited amount of time I had. Help would be appreciated, but it could be in a different PR I guess.

Can you try again make format on Mac, I added the obj-c files to the excluded directories.

adolfogc · 2017-11-19T17:13:07Z

.github/CONTRIBUTING.md


 Formatting can be performed automatically by calling `make format` from the `build/` directory.

+Note that [formatting can be disabled on a piece of code](https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code) if manual formatting is deemed more readable.


adolfogc · 2017-11-19T17:16:43Z

@louib I tested again using macOS, I can confirm the issue is now fixed.

adolfogc · 2017-11-19T17:27:46Z

@louib

I tried setting up clang-tidy but wasn't able to come up with a complete solution with the limited amount of time I had. Help would be appreciated, but it could be in a different PR I guess.

Sure, we can work on it in a next PR.

louib · 2017-11-19T17:29:08Z

@adolfogc cool, thanks!

If you guys are good with the solution we can keep this PR open until before the next major release and then format the codebase and merge.

I'd still like to have @droidmonkey 's approval since he wasn't convinced by the brute-force approach of enforcing formatting. My rationale for using git diff is that contributors can actually see in travis which sections of the code are not formatted properly and thus know that it's a formatting problem.

TheZ3ro

For me it's an approval. Leaving the PR open until all the code is formatted before 2.3.0

phoerious · 2017-11-21T00:20:32Z

I give my approval under the condition that we have a rough look through the code in advance and identify sections that need // clang-format off and submit them as addendum to this PR.

droidmonkey · 2017-11-21T03:14:51Z

I'm good with this approach as long as we make a massive reformat (that doesn't impact functionality) right before we complete 2.3.0

phoerious · 2018-01-13T23:06:19Z

After #1305 and #1327 are merged, we could do the reformatting. But this PR needs to be adjusted for TeamCity.

louib · 2018-01-14T15:45:47Z

@phoerious ping me when both PRs are merged, I'll do the formatting. Not sure how to do the TeamCity adjustment though.

louib · 2018-03-08T14:07:23Z

@phoerious I'll let you take care of this one.

louib · 2018-03-18T15:25:18Z

@phoerious rebased, changed base branch and reformatted the code.

droidmonkey · 2018-03-18T16:09:56Z

Do we still want to do this for 2.3.2 or 2.4 (ie develop)

TheZ3ro · 2018-03-19T10:55:33Z

Sincerely, I think 2.4 it's better

louib · 2018-03-31T16:24:08Z

@keepassxreboot/core-developers rebased to develop and removed exclusion of http/ directory.

TheZ3ro · 2018-03-31T16:39:30Z

I think we can merge this, everybody agrees?

droidmonkey · 2018-03-31T17:00:14Z

Lets bite the bullet and do it before we get too far down the road

TheZ3ro · 2018-03-31T19:53:33Z

Seems that new code was added with #1702, we need to re-update this PR now

louib · 2018-03-31T20:02:04Z

@TheZ3ro should be good now.

louib requested a review from a team November 17, 2017 18:04

louib added build system discussion labels Nov 17, 2017

adolfogc reviewed Nov 18, 2017

View reviewed changes

adolfogc reviewed Nov 19, 2017

View reviewed changes

TheZ3ro approved these changes Nov 19, 2017

View reviewed changes

phoerious approved these changes Nov 21, 2017

View reviewed changes

droidmonkey approved these changes Nov 21, 2017

View reviewed changes

louib added this to the v2.3.0 milestone Dec 3, 2017

louib removed the discussion label Jan 13, 2018

louib force-pushed the code_formatting branch 2 times, most recently from 2b3d14f to d494741 Compare January 14, 2018 15:17

phoerious changed the base branch from develop to release/2.3.0 February 1, 2018 20:42

phoerious force-pushed the code_formatting branch from ccd63f4 to ec680ea Compare February 28, 2018 15:36

phoerious modified the milestones: v2.3.1, v2.3.2 Mar 6, 2018

louib force-pushed the code_formatting branch 2 times, most recently from d8fea07 to a24d127 Compare March 18, 2018 15:23

louib changed the base branch from develop to release/2.3.2 March 18, 2018 15:23

louib modified the milestones: v2.3.2, v2.4.0 Mar 24, 2018

louib and others added 4 commits March 31, 2018 12:15

Add ClangFormat rules

07bc9a7

Use xargs to avoid long command lines

0b6eb3b

Add ClangFormat exceptions

3eb9170

Removed http/ from excluded dirs.

74efc57

louib changed the base branch from release/2.3.2 to develop March 31, 2018 16:22

louib force-pushed the code_formatting branch from a24d127 to 9897b09 Compare March 31, 2018 16:22

louib force-pushed the code_formatting branch from 9897b09 to 74efc57 Compare March 31, 2018 19:57

Formatting the code.

8324d03

TheZ3ro merged commit dd246f1 into keepassxreboot:develop Apr 2, 2018

weslly mentioned this pull request Apr 5, 2018

Fix build error on macOS #1814

Merged

phoerious mentioned this pull request Jun 11, 2018

Introduce synchronize merge method #1992

Merged

louib deleted the code_formatting branch June 14, 2018 21:42

c4rlo mentioned this pull request Jan 20, 2019

Enforce code formatting #2623

Closed


		Formatting can be performed automatically by calling `make format` from the `build/` directory.

		Note that [formatting can be disabled on a piece of code](https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code) if manual formatting is deemed more readable.

Uh oh!

Conversation

louib commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How has this been tested?

Uh oh!

TheZ3ro commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

louib commented Nov 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adolfogc commented Nov 18, 2017

Uh oh!

adolfogc commented Nov 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adolfogc commented Nov 18, 2017

The steps I followed:

The error I got:

Possible solution:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheZ3ro commented Nov 19, 2017

Uh oh!

louib commented Nov 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adolfogc commented Nov 19, 2017

Uh oh!

adolfogc commented Nov 19, 2017

Uh oh!

louib commented Nov 19, 2017

Uh oh!

TheZ3ro left a comment

Choose a reason for hiding this comment

Uh oh!

phoerious commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droidmonkey commented Nov 21, 2017

Uh oh!

phoerious commented Jan 13, 2018

Uh oh!

louib commented Jan 14, 2018

Uh oh!

louib commented Mar 8, 2018

Uh oh!

louib commented Mar 18, 2018

Uh oh!

droidmonkey commented Mar 18, 2018

Uh oh!

TheZ3ro commented Mar 19, 2018

Uh oh!

louib commented Mar 31, 2018

Uh oh!

TheZ3ro commented Mar 31, 2018

Uh oh!

droidmonkey commented Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheZ3ro commented Mar 31, 2018

Uh oh!

louib commented Mar 31, 2018

Uh oh!

Reviewers

louib commented Nov 17, 2017 •

edited

Loading

TheZ3ro commented Nov 17, 2017 •

edited

Loading

phoerious commented Nov 21, 2017 •

edited

Loading

droidmonkey commented Mar 31, 2018 •

edited

Loading