Re: [RFC PATCH] .clang-format: Remove conditional comments
On Thu, 2020-11-05 at 07:44 +0100, Miguel Ojeda wrote: > There are a few important new features: https://clang.llvm.org/docs/ClangFormatStyleOptions.html > - AlignConsecutiveMacros is probably one of the biggest one for the > kernel that we were missing so far. There's no control as to effective column nor sensible mechanism to avoid extremely long indents with a single exceptional entry. > - IndentPPDirectives and Some yes, mostly no. AlignEscapedNewlines: Generally the kernel uses column 72 but there's not real consistency. clang-format doesn't have that option as far as I can tell. > Then there are a few others that pertain to us too: > - SpaceBeforeSquareBrackets no > - SpacesInConditionalStatement no > - SpaceAfterLogicalNot no > - SpaceInEmptyBlock no > - IndentGotoLabels no > > Others are also worth checking to see if we can take advantage of them: > - IncludeBlocks (and configuring IncludeCategories etc.) Might be worthwhile. It's different by maintainer preference though. Reverse Xmas tree is somewhat common in networking, (which I think is silly, but DaveM likes it). Some like alphabetic ordering, others by order of include. > - StatementMacros Kernel is not c++ so this is irrelevant for gcc macro statement expressions. > Then there are others that are not related to us, but to be consistent > we would explicitly set them in the file. Finally, for extra points, > we could already document the new ones in LLVM 11 if any, for the > future, but that is optional. > > If no one is up for the task, I will eventually do it... :-) Enjoy...
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Thu, Nov 5, 2020 at 1:35 AM Nick Desaulniers wrote: > > I'll help you paint your bikeshed. Oh, but what color? I see a red > door, and I want it painted black. > > Sounds to me like Miguel could win over a critic by addressing some of > your (quite valid) concerns. ;) I am happy to take the patch, but we need to at least enable other features that were added meanwhile -- that is the advantage of increasing the minimum! If someone wants to take a look (wink, wink -- Joe, you are very experienced with all the different styles used around the kernel and would be great to have you on board with clang-format), they can use the following list for starters. There are a few important new features: - AlignConsecutiveMacros is probably one of the biggest one for the kernel that we were missing so far. - IndentPPDirectives and AlignEscapedNewlines would be very good to use too -- but the kernel style is not consistent AFAIK, so we would need to decide. Then there are a few others that pertain to us too: - SpaceBeforeSquareBrackets - SpacesInConditionalStatement - SpaceAfterLogicalNot - SpaceInEmptyBlock - IndentGotoLabels Others are also worth checking to see if we can take advantage of them: - IncludeBlocks (and configuring IncludeCategories etc.) - StatementMacros Then there are others that are not related to us, but to be consistent we would explicitly set them in the file. Finally, for extra points, we could already document the new ones in LLVM 11 if any, for the future, but that is optional. If no one is up for the task, I will eventually do it... :-) Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Thu, Nov 5, 2020 at 1:33 AM Nick Desaulniers wrote: > > Yeah, that's annoying. Why don't you send me a patch that downgrades > it from hard error to polite warning (in case someone made a typo), > and I'll review it? Sure! Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Tue, Nov 3, 2020 at 5:31 PM Joe Perches wrote: > > On Tue, 2020-11-03 at 17:08 -0800, Nick Desaulniers wrote: > > On Tue, Nov 3, 2020 at 1:33 PM Miguel Ojeda > > wrote: > > > On Tue, Nov 3, 2020 at 7:29 PM Joe Perches wrote: > > > > > > > > Now that the clang minimum supported version is > 10.0, enable the > > > > commented out conditional reformatting key:value lines in the file. > > > > > > > > Signed-off-by: Joe Perches > > > > --- > > > > > > > > Hey Miguel. > > > > > > > > I don't use this, but on its face it seems a reasonable change > > > > if the commented out key:value lines are correct. > > > > Joe, > > what would it take to get you to use clang-format, or at least try it? > > Beers? Bribes? Dirty deeds, done dirt cheap? > > Hey Nick. > > Paint my house? ;) I'll help you paint your bikeshed. Oh, but what color? I see a red door, and I want it painted black. Sounds to me like Miguel could win over a critic by addressing some of your (quite valid) concerns. ;) -- Thanks, ~Nick Desaulniers
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Tue, Nov 3, 2020 at 7:40 PM Miguel Ojeda wrote: > > distro etc.). One of the reasons for that was to help adoption, as > well as because clang-format gives a hard error on encountering an > unknown option :-( Yeah, that's annoying. Why don't you send me a patch that downgrades it from hard error to polite warning (in case someone made a typo), and I'll review it? -- Thanks, ~Nick Desaulniers
Re: [RFC PATCH] .clang-format: Remove conditional comments
Hi Joe, First of all, thanks for taking the time to write your reasoning. On Wed, Nov 4, 2020 at 5:17 AM Joe Perches wrote: > > The current kernel is v5.10 which requires clang 10.0 or higher. For building, yes. > This patch is not to be applied or backported to old kernels so no > person is going to use this patch on any old or backported kernel. Agreed (see my answer to Nick). > If a person is going to use clang-format on the current kernel sources > unless they are developing for the current kernel. > > They are going to have to be using clang 10.0 or higher and therefore > also will have and be using clang-format 10.0 or higher. No, they might be using GCC as usual and installed clang-format from their distro. In fact, I'd expect most developers accustomed to GCC to try it out that way, and also most of them to install compilers from their distro, not from the webpage, unless they need a newer version for some reason (e.g. new warnings, new debugging features in the kernel, etc.). In principle, clang-format (as a tool) is not related to building the kernel. We may call it "x-format" and think about it as a statically linked binary. What I am saying is that aligning clang-format to LLVM (now that LLVM has a minimum supported version) is not a necessity. We can still do it, of course, since there are new features for everyone and anybody that complains can install a newer version from the webpage. But there is nothing that forces us to require it. It is a decision that we balance w.r.t. new features. To put it concretely: if there were 0 new features or big fixes in clang-format 10 compared to 4, there would be no reason whatsoever to require users to download a new version. On the other side of the spectrum, some projects require a concrete version (not just a minimum), because they automatically format their entire codebase and want to avoid differences in output between clang-format versions. But we are far from automatically formatting the entire codebase. > Take it or not, apply it or not. I don't use clang-format and unless > there are improvements to it, I imagine I'll continue to use emacs > indent-region and a few other reformatting tools instead. Again, I am not opposed to the change. In fact, I am eager to improve the output of clang-format so that more people get engaged with it. I was just surprised you asserted so strongly that nobody is using clang-format from their distro. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Wed, 2020-11-04 at 04:57 +0100, Miguel Ojeda wrote: > On Wed, Nov 4, 2020 at 4:15 AM Joe Perches wrote: > > > > No one ever will use clang-format on the current kernel sources > > without having a recent version of clang and clang-format. > > Why? Many distros come with clang-format pre-packaged, and in fact the > original patch (that you commented on) argued for the >= 4 requirement > that way. The current kernel is v5.10 which requires clang 10.0 or higher. This patch is for the current kernel. This patch is not to be applied or backported to old kernels so no person is going to use this patch on any old or backported kernel. If a person is going to use clang-format on the current kernel sources unless they are developing for the current kernel. They are going to have to be using clang 10.0 or higher and therefore also will have and be using clang-format 10.0 or higher. Take it or not, apply it or not. I don't use clang-format and unless there are improvements to it, I imagine I'll continue to use emacs indent-region and a few other reformatting tools instead. cheers, Joe
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Wed, Nov 4, 2020 at 4:15 AM Joe Perches wrote: > > No one ever will use clang-format on the current kernel sources > without having a recent version of clang and clang-format. Why? Many distros come with clang-format pre-packaged, and in fact the original patch (that you commented on) argued for the >= 4 requirement that way. I am not saying we cannot change it, in fact I'd prefer if we do it (see my answer to Nick); but I don't understand your basis to claim nobody is installing their distro clang-format package. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Wed, Nov 4, 2020 at 2:08 AM Nick Desaulniers wrote: > > Miguel, > Really? :P I'd bet if you picked up this patch no one would notice. > > I recommend a simpler approach to multiple version support, which is > just matching the one version recommended for the rest of LLVM tools. > Sure, technically you can use older tools, but do so at your own peril > and don't complain to us if it doesn't work. Otherwise trying to Originally, the .clang-format file was made to work with old versions in order to make it easy for people to use (just install it from your distro etc.). One of the reasons for that was to help adoption, as well as because clang-format gives a hard error on encountering an unknown option :-( I am not opposed to changing those requirements now and say it is part of the LLVM toolchain (even if it is independent from building), but somebody might be annoyed. > explain different versions and even for different directories gets way > too complex for anyone to take seriously. That is just an escape hatch for developers that really need the latest formatting options (e.g. to minimize the exceptions in fully formatted files) or temporarily deal with some bits of kernel code with a different style. I definitely wouldn't want people adding their own .clang-format files without good reason... > It's not like we backport raising the minimum version. That is a good point. In fact, we can just do it very early in the cycle and wait to see who complains. If there are too many complaints, we can always revert it. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Wed, 2020-11-04 at 04:10 +0100, Miguel Ojeda wrote: > On Wed, Nov 4, 2020 at 1:56 AM Joe Perches wrote: > > > > Do remember that this patch is for the current kernel and > > not any old version that someone might be compiling. > > > > The current kernel _requires_ clang 10.0+ and that would > > obviously provide clang-format 10+ as well. > > You can use clang-format without having ever built a kernel with Clang. No one ever will use clang-format on the current kernel sources without having a recent version of clang and clang-format.
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Wed, Nov 4, 2020 at 1:56 AM Joe Perches wrote: > > Do remember that this patch is for the current kernel and > not any old version that someone might be compiling. > > The current kernel _requires_ clang 10.0+ and that would > obviously provide clang-format 10+ as well. You can use clang-format without having ever built a kernel with Clang. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Tue, 2020-11-03 at 17:08 -0800, Nick Desaulniers wrote: > On Tue, Nov 3, 2020 at 1:33 PM Miguel Ojeda > wrote: > > On Tue, Nov 3, 2020 at 7:29 PM Joe Perches wrote: > > > > > > Now that the clang minimum supported version is > 10.0, enable the > > > commented out conditional reformatting key:value lines in the file. > > > > > > Signed-off-by: Joe Perches > > > --- > > > > > > Hey Miguel. > > > > > > I don't use this, but on its face it seems a reasonable change > > > if the commented out key:value lines are correct. > > Joe, > what would it take to get you to use clang-format, or at least try it? > Beers? Bribes? Dirty deeds, done dirt cheap? Hey Nick. Paint my house? ;) I've tried it. It's OK. It's not significantly better than Lindent in some ways, in other ways it's pretty good. It can make a real hash though of well formatted, human readable code. I think that's it's biggest drawback. I posted an example of it a year or so back. https://lore.kernel.org/lkml/e9cb9bc8bd7fe38a5bb6ff7b7222b512acc7b018.ca...@perches.com/ In that thread I wrote: On Thu, 2019-09-12 at 03:18 -0700, Joe Perches wrote: > On Thu, 2019-09-12 at 10:24 +0200, Miguel Ojeda wrote: > > On Thu, Sep 12, 2019 at 9:43 AM Dan Williams > > wrote: > > > Now I come to find that CodingStyle has settled on clang-format (in > > > the last 15 months) as the new standard which is a much better answer > > > to me than a manually specified style open to interpretation. I'll > > > take a look at getting libnvdimm converted over. > > > > Note that clang-format cannot do everything as we want within the > > kernel just yet, but it is a close enough approximation -- it is near > > the point where we could simply agree to use it and stop worrying > > about styling issues. However, that would mean everyone needs to have > > a recent clang-format available, which I think is the biggest obstacle > > at the moment. > > I don't think that's close to true yet for clang-format. > > For instance: clang-format does not do anything with > missing braces, or coalescing multi-part strings, > or any number of other nominal coding style defects > like all the for_each macros, aligning or not aligning > columnar contents appropriately, etc... > > clang-format as yet has no taste. > > I believe it'll take a lot of work to improve it to a point > where its formatting is acceptable and appropriate. > > An AI rather than a table based system like clang-format is > more likely to be a real solution, but training that AI > isn't a thing that I want to do. and an example very poor conversion from that same thread: unsigned int key, newkey; int i; - rc = sscanf(buf, "%"__stringify(SEC_CMD_SIZE)"s" - " %"__stringify(KEY_ID_SIZE)"s" - " %"__stringify(KEY_ID_SIZE)"s", - cmd, keystr, nkeystr); + rc = sscanf( + buf, + "%" __stringify( + SEC_CMD_SIZE) "s" + " %" __stringify( + KEY_ID_SIZE) "s" + " %" __stringify( + KEY_ID_SIZE) "s", + cmd, keystr, nkeystr);
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Tue, Nov 3, 2020 at 10:29 AM Joe Perches wrote: > > Now that the clang minimum supported version is > 10.0, enable the > commented out conditional reformatting key:value lines in the file. > > Signed-off-by: Joe Perches > --- > > Hey Miguel. > > I don't use this, but on its face it seems a reasonable change > if the commented out key:value lines are correct. Acked-by: Nick Desaulniers -- Thanks, ~Nick Desaulniers
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Tue, Nov 3, 2020 at 1:33 PM Miguel Ojeda wrote: > > Hi Joe, > > On Tue, Nov 3, 2020 at 7:29 PM Joe Perches wrote: > > > > Now that the clang minimum supported version is > 10.0, enable the > > commented out conditional reformatting key:value lines in the file. > > > > Signed-off-by: Joe Perches > > --- > > > > Hey Miguel. > > > > I don't use this, but on its face it seems a reasonable change > > if the commented out key:value lines are correct. Joe, what would it take to get you to use clang-format, or at least try it? Beers? Bribes? Dirty deeds, done dirt cheap? > It is, yeah; however, the concern is that there may be developers > running an old clang-format from their distro (i.e. not using it for > compiling the kernel). We need to compare the functionality advantage > vs. the inconvenience of installing a current LLVM. The best would be > to ask whoever is using it right now, but there is no easy way to do > that -- many will only notice when the change is actually pushed :-) > > So far, I have avoided upgrading the requirement until clang-format > could match the kernel style even better (i.e. so that when the > upgrade happens, there is a reason for it). Also, the configuration > can be overridden in subfolders, thus a maintainer can push things > forward in a subsystem meanwhile. Miguel, Really? :P I'd bet if you picked up this patch no one would notice. I recommend a simpler approach to multiple version support, which is just matching the one version recommended for the rest of LLVM tools. Sure, technically you can use older tools, but do so at your own peril and don't complain to us if it doesn't work. Otherwise trying to explain different versions and even for different directories gets way too complex for anyone to take seriously. It's not like we backport raising the minimum version. I was very much in denial of committing to a relatively high minimum version of LLVM myself, until Linus recommended the simpler approach which folks voted in favor of at Plumbers. Maybe not a perfect analogy though. -- Thanks, ~Nick Desaulniers
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Tue, 2020-11-03 at 22:33 +0100, Miguel Ojeda wrote: > It is, yeah; however, the concern is that there may be developers > running an old clang-format from their distro (i.e. not using it for > compiling the kernel). I expect that'd be extremely unusual. > We need to compare the functionality advantage > vs. the inconvenience of installing a current LLVM. The best would be > to ask whoever is using it right now, but there is no easy way to do > that -- many will only notice when the change is actually pushed :-) Do remember that this patch is for the current kernel and not any old version that someone might be compiling. The current kernel _requires_ clang 10.0+ and that would obviously provide clang-format 10+ as well.
Re: [RFC PATCH] .clang-format: Remove conditional comments
Hi Joe, On Tue, Nov 3, 2020 at 7:29 PM Joe Perches wrote: > > Now that the clang minimum supported version is > 10.0, enable the > commented out conditional reformatting key:value lines in the file. > > Signed-off-by: Joe Perches > --- > > Hey Miguel. > > I don't use this, but on its face it seems a reasonable change > if the commented out key:value lines are correct. It is, yeah; however, the concern is that there may be developers running an old clang-format from their distro (i.e. not using it for compiling the kernel). We need to compare the functionality advantage vs. the inconvenience of installing a current LLVM. The best would be to ask whoever is using it right now, but there is no easy way to do that -- many will only notice when the change is actually pushed :-) So far, I have avoided upgrading the requirement until clang-format could match the kernel style even better (i.e. so that when the upgrade happens, there is a reason for it). Also, the configuration can be overridden in subfolders, thus a maintainer can push things forward in a subsystem meanwhile. Cheers, Miguel