|
From: Paul F. <pa...@so...> - 2023-04-17 20:07:17
|
https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=1b3430761f6bda43b8187dbd342b34cb5c99df3f commit 1b3430761f6bda43b8187dbd342b34cb5c99df3f Author: Paul Floyd <pj...@wa...> Date: Mon Apr 17 22:05:30 2023 +0200 Bug 468401 - [PATCH] Add a style file for clang-format Patch submitted by: Petr Pavlu <pet...@da...> Diff: --- .clang-format | 17 +++++++++++++++++ .gitignore | 1 + NEWS | 1 + README_DEVELOPERS | 18 ++++++++++++++++++ 4 files changed, 37 insertions(+) diff --git a/.clang-format b/.clang-format new file mode 100644 index 0000000000..4450e737ac --- /dev/null +++ b/.clang-format @@ -0,0 +1,17 @@ +--- +Language: Cpp +BasedOnStyle: LLVM + +AlignConsecutiveAssignments: true +AlignConsecutiveDeclarations: true +AlignConsecutiveMacros: true +AllowAllParametersOfDeclarationOnNextLine: true +BinPackParameters: false +BreakBeforeBraces: Linux +ContinuationIndentWidth: 3 +IndentWidth: 3 +PointerAlignment: Left +# Mark the VG_(), ML_() and tool macros as type declarations which they are +# sufficiently close to, otherwise clang-format gets confused by them. +TypenameMacros: [VG_, ML_, CLG_, DRD_, HG_, MC_] +... diff --git a/.gitignore b/.gitignore index a88ab4dd43..6622e7c59e 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ # / /.in_place /.vs +/.clang-format /acinclude.m4 /aclocal.m4 /autom4te-*.cache diff --git a/NEWS b/NEWS index 43ff9766bd..696720e97e 100644 --- a/NEWS +++ b/NEWS @@ -152,6 +152,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 467714 fdleak_* and rlimit tests fail when parent process has more than 64 descriptors opened 467839 Gdbserver: Improve compatibility of library directory name +468401 [PATCH] Add a style file for clang-format 468556 Build failure for vgdb n-i-bz FreeBSD rfork syscall fail with EINVAL or ENOSYS rather than VG_(unimplemented) diff --git a/README_DEVELOPERS b/README_DEVELOPERS index 9c04763d47..979ee13b4a 100644 --- a/README_DEVELOPERS +++ b/README_DEVELOPERS @@ -372,3 +372,21 @@ translated, and that includes the address. Then re-run with 999999 changed to the highest bb number shown. This will print the one line per block, and also will print a disassembly of the block in which the fault occurred. + + +Formatting the code with clang-format +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +clang-format is a tool to format C/C++/... code. The root directory of the +Valgrind tree contains file .clang-format which is a configuration for this tool +and specifies a style for Valgrind. This gives you an option to use +clang-format to easily format Valgrind code which you are modifying. + +The Valgrind codebase is not globally formatted with clang-format. It means +that you should not use the tool to format a complete file after making changes +in it because that would lead to creating unrelated modifications. + +The right approach is to format only updated or new code. By using an +integration with a text editor, it is possible to reformat arbitrary blocks +of code with a single keystroke. Refer to the upstream documentation which +describes integration with various editors and IDEs: +https://clang.llvm.org/docs/ClangFormat.html. |
|
From: Nicholas N. <n.n...@gm...> - 2023-04-18 03:30:26
|
Is there any appetite for clang-formatting Valgrind's code? I've now used auto-formatters in C++, Rust, and Python, and found it an excellent experience, and I get annoyed when working on code without auto-formatting. The C++ case was Firefox, where a large and old codebase was formatted. So it is possible (and better, IMO) to not just limit it to new code. It could certainly be done in pieces, e.g. one directory at a time, something like that. Nick On Tue, 18 Apr 2023 at 06:07, Paul Floyd <pa...@so...> wrote: > > https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=1b3430761f6bda43b8187dbd342b34cb5c99df3f > > commit 1b3430761f6bda43b8187dbd342b34cb5c99df3f > Author: Paul Floyd <pj...@wa...> > Date: Mon Apr 17 22:05:30 2023 +0200 > > Bug 468401 - [PATCH] Add a style file for clang-format > > Patch submitted by: > Petr Pavlu <pet...@da...> > > Diff: > --- > .clang-format | 17 +++++++++++++++++ > .gitignore | 1 + > NEWS | 1 + > README_DEVELOPERS | 18 ++++++++++++++++++ > 4 files changed, 37 insertions(+) > > diff --git a/.clang-format b/.clang-format > new file mode 100644 > index 0000000000..4450e737ac > --- /dev/null > +++ b/.clang-format > @@ -0,0 +1,17 @@ > +--- > +Language: Cpp > +BasedOnStyle: LLVM > + > +AlignConsecutiveAssignments: true > +AlignConsecutiveDeclarations: true > +AlignConsecutiveMacros: true > +AllowAllParametersOfDeclarationOnNextLine: true > +BinPackParameters: false > +BreakBeforeBraces: Linux > +ContinuationIndentWidth: 3 > +IndentWidth: 3 > +PointerAlignment: Left > +# Mark the VG_(), ML_() and tool macros as type declarations which they > are > +# sufficiently close to, otherwise clang-format gets confused by them. > +TypenameMacros: [VG_, ML_, CLG_, DRD_, HG_, MC_] > +... > diff --git a/.gitignore b/.gitignore > index a88ab4dd43..6622e7c59e 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -3,6 +3,7 @@ > # / > /.in_place > /.vs > +/.clang-format > /acinclude.m4 > /aclocal.m4 > /autom4te-*.cache > diff --git a/NEWS b/NEWS > index 43ff9766bd..696720e97e 100644 > --- a/NEWS > +++ b/NEWS > @@ -152,6 +152,7 @@ are not entered into bugzilla tend to get forgotten > about or ignored. > 467714 fdleak_* and rlimit tests fail when parent process has more than > 64 descriptors opened > 467839 Gdbserver: Improve compatibility of library directory name > +468401 [PATCH] Add a style file for clang-format > 468556 Build failure for vgdb > n-i-bz FreeBSD rfork syscall fail with EINVAL or ENOSYS rather than > VG_(unimplemented) > > diff --git a/README_DEVELOPERS b/README_DEVELOPERS > index 9c04763d47..979ee13b4a 100644 > --- a/README_DEVELOPERS > +++ b/README_DEVELOPERS > @@ -372,3 +372,21 @@ translated, and that includes the address. > Then re-run with 999999 changed to the highest bb number shown. > This will print the one line per block, and also will print a > disassembly of the block in which the fault occurred. > + > + > +Formatting the code with clang-format > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > +clang-format is a tool to format C/C++/... code. The root directory of > the > +Valgrind tree contains file .clang-format which is a configuration for > this tool > +and specifies a style for Valgrind. This gives you an option to use > +clang-format to easily format Valgrind code which you are modifying. > + > +The Valgrind codebase is not globally formatted with clang-format. It > means > +that you should not use the tool to format a complete file after making > changes > +in it because that would lead to creating unrelated modifications. > + > +The right approach is to format only updated or new code. By using an > +integration with a text editor, it is possible to reformat arbitrary > blocks > +of code with a single keystroke. Refer to the upstream documentation > which > +describes integration with various editors and IDEs: > +https://clang.llvm.org/docs/ClangFormat.html. > > > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers > |
|
From: Eyal S. <eya...@gm...> - 2023-04-18 03:41:47
|
The problem with doing this is that it really messes with the git blame, introducing a lot of changes! If you do this, you should probably add some sort of formatting check to a CI process somewhere, otherwise your work will get stale and you'll just be doing the clang-format again in a year from now. Good luck to you trying to get everyone to agree on a format! Ha! Eyal On Mon, Apr 17, 2023 at 9:31 PM Nicholas Nethercote <n.n...@gm...> wrote: > Is there any appetite for clang-formatting Valgrind's code? I've now used > auto-formatters in C++, Rust, and Python, and found it an excellent > experience, and I get annoyed when working on code without auto-formatting. > The C++ case was Firefox, where a large and old codebase was formatted. So > it is possible (and better, IMO) to not just limit it to new code. > > It could certainly be done in pieces, e.g. one directory at a time, > something like that. > > Nick > > On Tue, 18 Apr 2023 at 06:07, Paul Floyd <pa...@so...> wrote: > >> >> https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=1b3430761f6bda43b8187dbd342b34cb5c99df3f >> >> commit 1b3430761f6bda43b8187dbd342b34cb5c99df3f >> Author: Paul Floyd <pj...@wa...> >> Date: Mon Apr 17 22:05:30 2023 +0200 >> >> Bug 468401 - [PATCH] Add a style file for clang-format >> >> Patch submitted by: >> Petr Pavlu <pet...@da...> >> >> Diff: >> --- >> .clang-format | 17 +++++++++++++++++ >> .gitignore | 1 + >> NEWS | 1 + >> README_DEVELOPERS | 18 ++++++++++++++++++ >> 4 files changed, 37 insertions(+) >> >> diff --git a/.clang-format b/.clang-format >> new file mode 100644 >> index 0000000000..4450e737ac >> --- /dev/null >> +++ b/.clang-format >> @@ -0,0 +1,17 @@ >> +--- >> +Language: Cpp >> +BasedOnStyle: LLVM >> + >> +AlignConsecutiveAssignments: true >> +AlignConsecutiveDeclarations: true >> +AlignConsecutiveMacros: true >> +AllowAllParametersOfDeclarationOnNextLine: true >> +BinPackParameters: false >> +BreakBeforeBraces: Linux >> +ContinuationIndentWidth: 3 >> +IndentWidth: 3 >> +PointerAlignment: Left >> +# Mark the VG_(), ML_() and tool macros as type declarations which they >> are >> +# sufficiently close to, otherwise clang-format gets confused by them. >> +TypenameMacros: [VG_, ML_, CLG_, DRD_, HG_, MC_] >> +... >> diff --git a/.gitignore b/.gitignore >> index a88ab4dd43..6622e7c59e 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -3,6 +3,7 @@ >> # / >> /.in_place >> /.vs >> +/.clang-format >> /acinclude.m4 >> /aclocal.m4 >> /autom4te-*.cache >> diff --git a/NEWS b/NEWS >> index 43ff9766bd..696720e97e 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -152,6 +152,7 @@ are not entered into bugzilla tend to get forgotten >> about or ignored. >> 467714 fdleak_* and rlimit tests fail when parent process has more than >> 64 descriptors opened >> 467839 Gdbserver: Improve compatibility of library directory name >> +468401 [PATCH] Add a style file for clang-format >> 468556 Build failure for vgdb >> n-i-bz FreeBSD rfork syscall fail with EINVAL or ENOSYS rather than >> VG_(unimplemented) >> >> diff --git a/README_DEVELOPERS b/README_DEVELOPERS >> index 9c04763d47..979ee13b4a 100644 >> --- a/README_DEVELOPERS >> +++ b/README_DEVELOPERS >> @@ -372,3 +372,21 @@ translated, and that includes the address. >> Then re-run with 999999 changed to the highest bb number shown. >> This will print the one line per block, and also will print a >> disassembly of the block in which the fault occurred. >> + >> + >> +Formatting the code with clang-format >> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> +clang-format is a tool to format C/C++/... code. The root directory of >> the >> +Valgrind tree contains file .clang-format which is a configuration for >> this tool >> +and specifies a style for Valgrind. This gives you an option to use >> +clang-format to easily format Valgrind code which you are modifying. >> + >> +The Valgrind codebase is not globally formatted with clang-format. It >> means >> +that you should not use the tool to format a complete file after making >> changes >> +in it because that would lead to creating unrelated modifications. >> + >> +The right approach is to format only updated or new code. By using an >> +integration with a text editor, it is possible to reformat arbitrary >> blocks >> +of code with a single keystroke. Refer to the upstream documentation >> which >> +describes integration with various editors and IDEs: >> +https://clang.llvm.org/docs/ClangFormat.html. >> >> >> _______________________________________________ >> Valgrind-developers mailing list >> Val...@li... >> https://lists.sourceforge.net/lists/listinfo/valgrind-developers >> > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers > |
|
From: Nicholas N. <n.n...@gm...> - 2023-04-18 05:05:48
|
On Tue, 18 Apr 2023 at 13:41, Eyal Soha <eya...@gm...> wrote: > The problem with doing this is that it really messes with the git blame, > introducing a lot of changes! > That's always the first objection that is raised. Turns out there's a good solution <https://medium.com/codex/how-to-introduce-a-code-formatter-without-messing-up-git-history-4a16bd074c10> . > If you do this, you should probably add some sort of formatting check to a > CI process somewhere, otherwise your work will get stale and you'll just be > doing the clang-format again in a year from now. > Yes. > Good luck to you trying to get everyone to agree on a format! Ha! > It requires negotiation, but it's doable. Remember, all of this was done successfully for Firefox, which is a much bigger and gnarlier codebase than Valgrind. Nick |
|
From: Paul F. <pj...@wa...> - 2023-04-18 06:48:38
|
On 18-04-23 05:41, Eyal Soha wrote:
> The problem with doing this is that it really messes with the git blame,
> introducing a lot of changes!
>
> If you do this, you should probably add some sort of formatting check to
> a CI process somewhere, otherwise your work will get stale and you'll
> just be doing the clang-format again in a year from now.
My goal would be to have some sort of git trigger that maintains formatting.
> Good luck to you trying to get everyone to agree on a format! Ha!
I think that there the battle is mostly won. There are informal "house
rules" and most of the code is 3 space indented with cuddle braces.
There is still a bit of variation (e.g., whether braces indent after
switch or not).
It is also possible to 'protect' blocks of code from clang-format e.g.,
int formatted_code;
// clang-format off
void unformatted_code ;
// clang-format on
(copied from https://clang.llvm.org/docs/ClangFormatStyleOptions.html)
A+
Paul
|
|
From: Nicholas N. <n.n...@gm...> - 2023-04-18 07:28:14
|
If I had a single vote for a single element of a new style, it would be to change the 3 space indents to either 4 or 2 :) Nick On Tue, 18 Apr 2023 at 16:49, Paul Floyd <pj...@wa...> wrote: > > > On 18-04-23 05:41, Eyal Soha wrote: > > The problem with doing this is that it really messes with the git blame, > > introducing a lot of changes! > > > > If you do this, you should probably add some sort of formatting check to > > a CI process somewhere, otherwise your work will get stale and you'll > > just be doing the clang-format again in a year from now. > > My goal would be to have some sort of git trigger that maintains > formatting. > > > Good luck to you trying to get everyone to agree on a format! Ha! > > I think that there the battle is mostly won. There are informal "house > rules" and most of the code is 3 space indented with cuddle braces. > There is still a bit of variation (e.g., whether braces indent after > switch or not). > > It is also possible to 'protect' blocks of code from clang-format e.g., > > > int formatted_code; > // clang-format off > void unformatted_code ; > // clang-format on > > (copied from https://clang.llvm.org/docs/ClangFormatStyleOptions.html) > > A+ > Paul > > > > _______________________________________________ > Valgrind-developers mailing list > Val...@li... > https://lists.sourceforge.net/lists/listinfo/valgrind-developers > |
|
From: Paul F. <pj...@wa...> - 2023-04-18 07:14:34
|
On 18-04-23 05:30, Nicholas Nethercote wrote: > Is there any appetite for clang-formatting Valgrind's code? At work, we have numerous projects that have been worked on by large numbers of people with probably just about every imaginable free code editor. And over 30 years or more. Some teams and sub-projects have fairly consistent formatting. Others have random mixes of tabs and spaces. It drives me mad (it also drives clang-tidy and gcc mad with lots of warnings about inconsistent indentation). All that to say is that I'm in favour of using clang-format, A+ Paul |
|
From: Julian S. <jse...@gm...> - 2023-04-18 07:32:11
|
On 18/04/2023 09:14, Paul Floyd wrote: > On 18-04-23 05:30, Nicholas Nethercote wrote: >> Is there any appetite for clang-formatting Valgrind's code? > All that to say is that I'm in favour of using clang-format, Me too; +1 for that. I've lived with the Firefox C++ auto-format stuff for some years now and that has worked out well. In hindsight I'd change the 3 char indents to 2; 2 works well for Fx, and maintain a max width of 80. J |
|
From: Mark W. <ma...@kl...> - 2023-04-20 20:02:59
|
Hi, On Tue, Apr 18, 2023 at 03:05:28PM +1000, Nicholas Nethercote wrote: > On Tue, 18 Apr 2023 at 13:41, Eyal Soha <eya...@gm...> wrote: > > > The problem with doing this is that it really messes with the git blame, > > introducing a lot of changes! > > That's always the first objection that is raised. Turns out there's a good > solution > <https://medium.com/codex/how-to-introduce-a-code-formatter-without-messing-up-git-history-4a16bd074c10> > . (Note that article requires javascript to turn on, or you won't be able to read more than the first paragraph.) Sure you can work around it, but I don't think that is a great solution. It requires everyone to make some local changes. > > If you do this, you should probably add some sort of formatting check to a > > CI process somewhere, otherwise your work will get stale and you'll just be > > doing the clang-format again in a year from now. > > > > Yes. > > > Good luck to you trying to get everyone to agree on a format! Ha! > > > > It requires negotiation, but it's doable. > > Remember, all of this was done successfully for Firefox, which is a much > bigger and gnarlier codebase than Valgrind. I am not a fan, but also not dead against. Personally I am happy with emacs M-x indent-region on the code I edit. There is a .dir-locals.el in git which catches some (but certainly not all) formatting things. I played a bit with the .clang-format file. I am not sure I like using clang-format to reformat everything, it seems a little arbitrary. But if we could make the git-clang-format thing working then using that for formatting patches/regions that you changed might be OK. Cheers, Mark |
|
From: Bart V. A. <bva...@ac...> - 2023-04-20 20:23:04
|
On 4/17/23 20:30, Nicholas Nethercote wrote: > Is there any appetite for clang-formatting Valgrind's code? Why to reformat the entire code base? Auto-formatting can be used without reformatting the entire code base. This is how I reformat Linux kernel and Android patches before I publish these: git clang-format HEAD^ git diff # to review the changes git commit -a --amend --no-edit # to keep the changes git reset --hard # to discard the changes Plugins are available for many editors (including Emacs and vi) that support clang-format. Bart. |
|
From: Paul F. <pj...@wa...> - 2023-04-22 09:55:24
|
On 20-04-23 22:22, Bart Van Assche wrote: > On 4/17/23 20:30, Nicholas Nethercote wrote: >> Is there any appetite for clang-formatting Valgrind's code? > > Why to reformat the entire code base? Auto-formatting can be used > without reformatting the entire code base. This is how I reformat Linux > kernel and Android patches before I publish these: > > git clang-format HEAD^ > git diff # to review the changes > git commit -a --amend --no-edit # to keep the changes > git reset --hard # to discard the changes > > Plugins are available for many editors (including Emacs and vi) that > support clang-format. Yes, Qt Creator (and I expect VS code as well) has ways to beautify a file, selected text, from the current line and to insert markup to disable clang-format. I expect that's mainly how I'll be using it (on selected blocks of code). A+ Paul |
|
From: Nicholas N. <n.n...@gm...> - 2023-04-20 20:52:09
|
On Fri, 21 Apr 2023 at 06:02, Mark Wielaard <ma...@kl...> wrote: > > Sure you can work around it, but I don't think that is a great > solution. It requires everyone to make some local changes. > You can send up a .gitconfig for the project, so it'll work automatically for everyone. > I am not a fan, but also not dead against. > Have you ever worked on a project that uses auto-formatting? Skepticism followed by enthusiasm is common. Paul, Julian, and I all have used it on other codebases and are now advocates. I'm using it for Cachegrind's Python code right now. It makes life easier. Personally I am happy with emacs M-x indent-region on the code I edit. > There is a .dir-locals.el in git which catches some (but certainly not > all) formatting things. > What about people who don't use emacs? Nick |
|
From: Bart V. A. <bva...@ac...> - 2023-04-20 21:06:51
|
On 4/20/23 13:51, Nicholas Nethercote wrote: > On Fri, 21 Apr 2023 at 06:02, Mark Wielaard <ma...@kl... > <mailto:ma...@kl...>> wrote: > I am not a fan, but also not dead against. > > Have you ever worked on a project that uses auto-formatting? Skepticism > followed by enthusiasm is common. Paul, Julian, and I all have used it > on other codebases and are now advocates. I'm using it for Cachegrind's > Python code right now. It makes life easier. I have worked on large projects that use auto-formatting. Despite this I'm strongly opposed against reformatting existing code. No matter how much time is spent on tuning the .clang-format file, there will always be code for which the formatting is made worse by clang-format than the existing code. Bart. |
|
From: Nicholas N. <n.n...@gm...> - 2023-04-20 23:06:24
|
On Fri, 21 Apr 2023 at 07:06, Bart Van Assche <bva...@ac...> wrote: > No matter how > much time is spent on tuning the .clang-format file, there will always > be code for which the formatting is made worse by clang-format than the > existing code. > It's true that there are rare cases where an auto-formatter does a bad job, such as code in tabular form. Fortunately there's an easy workaround for that too: you just put `// clang-format off` at the start of the block and `// clang-format on` at the end of the block. Nick |
|
From: Mark W. <ma...@kl...> - 2023-04-22 17:25:23
|
Hi Nick,
On Fri, Apr 21, 2023 at 06:51:42AM +1000, Nicholas Nethercote wrote:
> On Fri, 21 Apr 2023 at 06:02, Mark Wielaard <ma...@kl...> wrote:
> > Sure you can work around it, but I don't think that is a great
> > solution. It requires everyone to make some local changes.
> >
>
> You can send up a .gitconfig for the project, so it'll work automatically
> for everyone.
How? If you can install the .gitconfig automatically that would be
interesting, but I believe you'll always have to install .gitconfig
tweaks by hand.
> > I am not a fan, but also not dead against.
>
> Have you ever worked on a project that uses auto-formatting? Skepticism
> followed by enthusiasm is common. Paul, Julian, and I all have used it on
> other codebases and are now advocates. I'm using it for Cachegrind's Python
> code right now. It makes life easier.
I can see how it would work nicely for new python code. gdb also uses
python black (plus a buildbot checker) to auto-format their python
code. If you like we could add the same to the buildbot for valgrind.
BTW. Trying the various formatters listed in auxprogs/pybuild.sh gives
me (pylint suggests to use a range generator instead of an array, then
black reformates the file to get everything on one line:
diff --git a/cachegrind/cg_annotate.in b/cachegrind/cg_annotate.in
index c76a760be..cba91aec7 100755
--- a/cachegrind/cg_annotate.in
+++ b/cachegrind/cg_annotate.in
@@ -1090,9 +1090,7 @@ def print_annotated_src_files(
# mtimes as if they are all as early as the earliest one.
# Therefore, we warn only if the earliest source file is
# more recent than the cgout file.
- min_ofl_st_mtime_ns = min(
- [os.stat(ofl).st_mtime_ns for ofl in ofls]
- )
+ min_ofl_st_mtime_ns = min(os.stat(ofl).st_mtime_ns for ofl in ofls)
for cgout_filename in args.cgout_filename:
if min_ofl_st_mtime_ns > os.stat(cgout_filename).st_mtime_ns:
> Personally I am happy with emacs M-x indent-region on the code I edit.
> > There is a .dir-locals.el in git which catches some (but certainly not
> > all) formatting things.
> >
>
> What about people who don't use emacs?
:) That sound like the start of a joke... I assume other editors have
something similar to M-X ident-region. But have no experience with
that. I just mentioned it to show that I do have a little experience
with formatters and do like valgrind having a .dir-locals.el. So if
the .clang-format gives the same kind of experience to others then
that is nice.
Cheers,
Mark
|
|
From: Nicholas N. <n.n...@gm...> - 2023-04-23 22:39:50
|
On Sun, 23 Apr 2023 at 03:25, Mark Wielaard <ma...@kl...> wrote: > > > > You can send up a .gitconfig for the project, so it'll work automatically > > for everyone. > > How? > We would have a file, called `.git-blame-ignore-revs` by convention. Here's an example that hides the auto-formatting/unformatting that Paul accidentally committed recently: # Accidental clang-format on a few files, 2023-04-22. > bf347551c99313a4af9c38bdeda9b946c9795945 > > # Undo the previous commit, 2023-04-22. > 76d6b4591a5a05e43e1a819bf630c0d8ee857a7e > I tested this out, it works well. Here is the vanilla blame for a couple of lines in `memcheck/mc_main.c` that were affected: 76d6b4591a (Paul Floyd 2023-04-22 16:29:27 +0200 60) static void > ocache_sarp_Set_Origins ( Addr, UWord, UInt ); /* fwds */ > 76d6b4591a (Paul Floyd 2023-04-22 16:29:27 +0200 61) static > void ocache_sarp_Clear_Origins ( Addr, UWord ); /* fwds */ > and here is the blame using the `.git-blame-ignore-revs` file: afebe61b37 (Nicholas Nethercote 2002-09-23 09:36:25 +0000 60) static void > ocache_sarp_Set_Origins ( Addr, UWord, UInt ); /* fwds */ > 4cae5c3ed5 (Julian Seward 2008-05-01 20:24:26 +0000 61) static > void ocache_sarp_Clear_Origins ( Addr, UWord ); /* fwds */ > To make that work seamlessly with `git blame` requires running `git config blame.ignoreRevsFile .git-blame-ignore-revs` once per cloned repository. That command could easily be put somewhere that will always run while building. Somewhere in `configure.ac` seems like a logical place though I'd be happy to hear suggestions otherwise. Firefox's `.git-blame-ignore-revs` file is here: https://searchfox.org/mozilla-central/source/.git-blame-ignore-revs. It has hundreds of entries! I expect/hope we wouldn't need anything like that many, but it shows that the solution scales nicely to very large codebases. The end result is that the effect of auto-formatting on history can be worked around in a straightforward manner. (BTW, we could take advantage of `configure.ac` to set up other git aliases. E.g. we could have one for pushing to a try branch, so you just have to run `git vgtry $BRANCH` instead of `git push origin $BRANCH:users/$USERNAME/try-$BRANCH`.) I can see how it would work nicely for new python code. gdb also uses > python black (plus a buildbot checker) to auto-format their python > code. If you like we could add the same to the buildbot for valgrind. > Sounds great. I am all for automating this kind of thing, making it impossible to forget to do it or do it incorrectly. How exactly would it work? Would it be a post-commit check? Could we get the same check to run locally, to run pre-commit? > BTW. Trying the various formatters listed in auxprogs/pybuild.sh gives > me (pylint suggests to use a range generator instead of an array, then > black reformates the file to get everything on one line: > > - min_ofl_st_mtime_ns = min( > - [os.stat(ofl).st_mtime_ns for ofl in ofls] > - ) > + min_ofl_st_mtime_ns = min(os.stat(ofl).st_mtime_ns > for ofl in ofls) Interesting. I have black 22.6.0, what version do you have? Nick |
|
From: Mark W. <ma...@kl...> - 2023-04-22 17:45:20
|
Hi Nick, On Fri, Apr 21, 2023 at 09:06:06AM +1000, Nicholas Nethercote wrote: > On Fri, 21 Apr 2023 at 07:06, Bart Van Assche <bva...@ac...> wrote: > > > No matter how > > much time is spent on tuning the .clang-format file, there will always > > be code for which the formatting is made worse by clang-format than the > > existing code. > > > > It's true that there are rare cases where an auto-formatter does a bad job, > such as code in tabular form. > > Fortunately there's an easy workaround for that too: you just put `// > clang-format off` at the start of the block and `// clang-format on` at the > end of the block. But why do you want to reformat all the exiting code? Isn't it enough if people have easy tools to format new hunks? I am happy if we encourage people to use code formatters for new/changed code. But forcing a reformat of the whole project on people seems like a bad idea. I think Paul's recent accident with clang-format shows why it is not a good idea to try to reformat any existing code. It just introduces huge arbitrary changes. Cheers, Mark |
|
From: Paul F. <pj...@wa...> - 2023-04-22 19:22:29
|
On 22-04-23 19:44, Mark Wielaard wrote: > > But why do you want to reformat all the exiting code? > Isn't it enough if people have easy tools to format new hunks? > > I am happy if we encourage people to use code formatters for > new/changed code. But forcing a reformat of the whole project on > people seems like a bad idea. > > I think Paul's recent accident with clang-format shows why it is not a > good idea to try to reformat any existing code. It just introduces > huge arbitrary changes. The main thing for me is that the current indentation isn't too bad. Things like line breaks, continuation, align with assignment, multiple statements on a line are less consistent. That also means bigger diffs if reformatting. I wasn't expecting the diffs to be so big. For now I don't see enough benefit for large scale reformatting, but I am planning to continue using it for new blocks of code and new files. A+ Paul |
|
From: Nicholas N. <n.n...@gm...> - 2023-04-26 10:32:26
|
On Sun, 23 Apr 2023 at 03:44, Mark Wielaard <ma...@kl...> wrote: > > But why do you want to reformat all the existing code? > Isn't it enough if people have easy tools to format new hunks? > > I am happy if we encourage people to use code formatters for > new/changed code. But forcing a reformat of the whole project on > people seems like a bad idea. > > I think Paul's recent accident with clang-format shows why it is not a > good idea to try to reformat any existing code. It just introduces > huge arbitrary changes. > They are not arbitrary changes, they are improvements. Auto-formatting the whole codebase has some major advantages. - Simplicity. "Format all C code with clang-format" is simpler than "Format new C code with clang-format, but leave old C code alone." And partial application can lead to weird results when you make a change that results in a tight interleaving of changed and unchanged lines. - Consistency. Instead of a mishmash of styles, everything is consistent. `foo ( bar )` vs `foo(bar)` is one prime example of an existing inconsistency, with both versions appearing all over the place. It makes me grumpy. - Readability. A lot of the current style choices are just bad. Some of the worst problems can't be fixed in a piecemeal fashion, e.g. three space indents are weird and terrible. There are some implicit principles that inform my opinions here. - The latest revision of the code is the most important revision. This means it's worth making changes to improve things. - Developer experience is important. Code should be enjoyable to work on. There are many aspects to this, and code formatting is one of them. "It's always been like this" is a weak justification for many things. As I understand it, there are two main objections to mass-reformatting. - It breaks `git blame`. But as we've seen, this is entirely fixable. - "If it ain't broken, don't fix it." But I argue that it the current inconsistent formatting is a form of brokenness. Of course some care is required to fix it, e.g. the particular style should be chosen with care, and diffs should be reviewed by humans and not just blindly accepted. But there are high-quality, widely-used tools that make it straightforward. We should use them. Nick |
|
From: Paul F. <pj...@wa...> - 2023-04-26 11:58:14
|
On 26-04-23 12:32, Nicholas Nethercote wrote: > > > As I understand it, there are two main objections to mass-reformatting. > > * It breaks `git blame`. But as we've seen, this is entirely fixable. > * "If it ain't broken, don't fix it." But I argue that it the current > inconsistent formatting is a form of brokenness. Of course some care > is required to fix it, e.g. the particular style should be chosen > with care, and diffs should be reviewed by humans and not just > blindly accepted. But there are high-quality, widely-used tools that > make it straightforward. We should use them. Hi Nick I'm much more in favour if the git blame history is maintained. Wrong indentation can be a source of errors. I haven't seen any in the Valgrind source, but more often these days I see "python++" code (C or C++ code that looks like Python). This is the one place where you need to be careful reviewing the diffs. (clang-tidy nags about this as well). A+ Paul |
|
From: Bart V. A. <bva...@ac...> - 2023-04-26 14:04:45
|
On 4/26/23 03:32, Nicholas Nethercote wrote:
> As I understand it, there are two main objections to mass-reformatting.
>
> * It breaks `git blame`. But as we've seen, this is entirely fixable.
> * "If it ain't broken, don't fix it." But I argue that it the
> current inconsistent formatting is a form of brokenness. Of course
> some care is required to fix it, e.g. the particular style should
> be chosen with care, and diffs should be reviewed by humans and
> not just blindly accepted. But there are high-quality, widely-used
> tools that make it straightforward. We should use them.
>
Are these the only disadvantages of reformatting the entire code base? I
see more disadvantages:
* Preserving the formatting of code that is in tabular format requires
manual annotation (/* clang-format off */ and /* clang-format on
*/). Who will do the tedious work of reviewing the entire code base
and annotating all the code for which formatting should be preserved?
* It is a certainty that reformatting the code with clang-format will
make formatting worse for a significant fraction of the code base. A
summary of the formatting made worse by clang-format for one
particular Linux kernel source file is available here: /Re: [PATCH]
scsi: megaraid: cleanup formatting of megaraid
<https://lore.kernel.org/linux-scsi/CAKwvOdnWHVV+3s8SO=Q8F...@ma.../>/.
* It is likely that the Valgrind .clang-format file will be modified
in the future. The Linux kernel .clang-format style file was
introduced in 2018 and since then has been modified 44 times:
$ git log .clang-format | grep -c ^commit
45
Will the entire Valgrind code base be reformatted every time the
Valgrind .clang-format file is modified?
Is the above list complete? Did I perhaps overlook any disadvantages?
Bart.
|
|
From: Nicholas N. <n.n...@gm...> - 2023-04-27 00:06:25
|
On Thu, 27 Apr 2023 at 00:04, Bart Van Assche <bva...@ac...> wrote: > Are these the only disadvantages of reformatting the entire code base? I > see more disadvantages: > > - Preserving the formatting of code that is in tabular format requires > manual annotation (/* clang-format off */ and /* clang-format on */). Who > will do the tedious work of reviewing the entire code base and annotating > all the code for which formatting should be preserved? > > I'm happy to start. I think doing the work in chunks makes sense, e.g. one subdirectory at a time, something like that. After that, others can contribute if they like. A prerequisite for that is deciding what rules to put in the .clang-format file. I am also happy to work on that. Some discussion, along with example diffs, will be necessary. > > - It is a certainty that reformatting the code with clang-format will > make formatting worse for a significant fraction of the code base. A > summary of the formatting made worse by clang-format for one particular > Linux kernel source file is available here: *Re: [PATCH] scsi: > megaraid: cleanup formatting of megaraid > <https://lore.kernel.org/linux-scsi/CAKwvOdnWHVV+3s8SO=Q8F...@ma.../>* > . > > That's a handful of quibbles. There will always be places where an auto-formatter does something that somebody doesn't like. That is also true when humans format code. > > - It is likely that the Valgrind .clang-format file will be modified > in the future. The Linux kernel .clang-format style file was introduced in > 2018 and since then has been modified 44 times: > > $ git log .clang-format | grep -c ^commit > 45 > > Will the entire Valgrind code base be reformatted every time the > Valgrind .clang-format file is modified? > > I expect .clang-format changes to be rare and minimal, and some care will be needed when deciding what rules to put in the initial .clang-format file. Changing the indent size every six months would be silly, of course. But to answer the question: absolutely. Because the goal here is to make auto-formatting mandatory. Any code that isn't formatted properly would be rejected. So if the style changes, the code changes with it. (Mandatory pre-commit testing, another modern software development convenience that Valgrind currently lacks, would help with this.) As for the Linux example: the "modified 44 times" is highly misleading. The version history of the file can be seen here <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/.clang-format>. The file currently has 688 lines, but 81% of those lines are for macros listed in the `ForEachMacros <https://clang.llvm.org/docs/ClangFormatStyleOptions.html#foreachmacros>` rule, which is a list of macros that needs particular treatment. Most of the 44 modifications are just modifications of that list, which are trivial and won't affect any code other than those macros, and aren't relevant for Valgrind. There were only five commits with meaningful changes that didn't involve that list: one, <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=c90f3b8c4b5b7ff3ee7103640a2ab1c960c69645> two <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=c90f3b8c4b5b7ff3ee7103640a2ab1c960c69645>, three <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=96232c7d4f847a5e597177236159e6b32ccf60e4>, four <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=d7f6604341c748f803810664d5603af22b84a8cc>, five <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/.clang-format?id=781121a7f6d11d7cae44982f174ea82adeec7db0>. Only one of those (the third one) changes more than one rule, and that was part of the upgrade from clang-format 4 to 6. clang-format is more mature now so changes of that sort are less likely to be needed in the future. > > > Is the above list complete? Did I perhaps overlook any disadvantages? > Auto-formatting is very common these days, to the point of being standard, and blanket opposition to auto-formatting has become a minority position. Which makes sense, because many projects have style guides, and automatic enforcement of style is better than manual enforcement, being less work and more reliable. Valgrind's development processes need to move with the times. Nick |
|
From: Bart V. A. <bva...@ac...> - 2023-04-27 17:08:21
|
On 4/26/23 17:06, Nicholas Nethercote wrote: > Auto-formatting is very common these days, to the point of being standard, and blanket opposition to auto-formatting has become a minority position. Really? Can you name one major open source project written in C or C++, that is not controlled by Mozilla and that enforces the formatting of the code by rejecting push requests that do not comply with the code style of that project? Bart. |
|
From: Nicholas N. <n.n...@gm...> - 2023-04-28 06:08:27
|
On Fri, 28 Apr 2023 at 03:08, Bart Van Assche <bva...@ac...> wrote: > > Can you name one major open source project written in C or C++, that is > not controlled by Mozilla and that enforces the formatting of the code by > rejecting push requests that do not comply with the code style of that > project? > MongoDB <https://github.com/mongodb/mongo/wiki/Server-Code-Style>, Blender <https://wiki.blender.org/wiki/Tools/ClangFormat>, Godot <https://docs.godotengine.org/en/stable/contributing/development/code_style_guidelines.html> all mandate the use of clang-format. (I didn't check whether they enforce this with a pre-push check, but the language in the linked documents makes it clear that auto-formatting is mandatory.) There's no good reason to ignore other languages, though. I know for certain that mandatory autoformatting is ubiquitous in Rust projects, with rustc and Cargo being obvious examples. I suspect there are countless Go and Swift projects that use it too. Nick |