<div dir="ltr"><div dir="ltr">On Tue, Aug 10, 2021 at 2:35 PM Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Aug 10, 2021 at 7:37 AM Manuel Klimek via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
><br>
> Fwiw, I think "clang-format can make breaking changes to code when we consider the benefit to be worth it" has been the policy on clang-format for a very long time, so accepting that as the official policy is IMO not a change. If somebody wants to write it down to prevent future revisiting, that seems fine with me.<br>
<br>
FWIW, I've never had the impression that this was the status quo for<br>
the tool. I've looked through the mailing list archives and some old<br>
reviews and I don't see any supporting evidence one way or the other<br>
(obviously I could have missed something though!).<br></blockquote><div><br></div><div>I agree that that was probably not obvious or stated very clearly anywhere - clang-format for years was basically developed by a small team without much interest in input by the rest of the world, for a very specific use case.</div><div>At the time we did not do a great job at spreading our thoughts with the world, as a lot of discussions were in person.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
~Aaron<br>
<br>
><br>
> On Tue, Aug 10, 2021 at 11:32 AM Björn Schäpers via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br>
>><br>
>> I'm all in favor of allowing such changes and will help to create and review<br>
>> these changes.<br>
>><br>
>> Kind regards,<br>
>> Björn (HazardyKnusperkeks).<br>
>><br>
>> Am 10.08.2021 um 10:32 schrieb MyDeveloper Day via cfe-dev:<br>
>> > Thanks for the response Sam,<br>
>> ><br>
>> > Here is how I see we mitigate the risk:<br>
>> ><br>
>> > On Mon, Aug 9, 2021 at 11:23 PM Sam McCall <<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a><br>
>> > <mailto:<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a>>> wrote:<br>
>> ><br>
>> > I'm cautiously +1 on const reordering, having previously opposed it and been<br>
>> > convinced.<br>
>> > I think anyone who's worked on a large shared codebase both before and after<br>
>> > clang-format can understand the value here, so I'll focus mostly on the<br>
>> > risks and why I think they're acceptable.<br>
>> ><br>
>> > *Risk: *clang-format will become a grab-bag of features with no clear line -<br>
>> > just anything implemented on top of its pseudo-AST.<br>
>> > Clang-format's brand is low-level formatting details and I think it's<br>
>> > important to preserve this. Const order fits here in users' minds. (So does<br>
>> > brace addition/removal).<br>
>> ><br>
>> ><br>
>> > I doubt we wouldn't continue to apply the same level of scrutiny on the code<br>
>> > reviews and expect them to follow best practices and guidelines, I am expecting<br>
>> > us to still be quite circumspect as to what we'd consider.<br>
>> ><br>
>> > To be honest clang-format I think already runs at quite a high review rejection<br>
>> > rate, people ask for all sorts of things and we do try to push back pretty hard,<br>
>> > landing something can sometimes be pretty torturous to get through review,<br>
>> > I'm not expecting that to change.<br>
>> ><br>
>> ><br>
>> > *Risk*: The feature will break code and clang-format will no longer be (seen<br>
>> > as) reliable. This can make it harder socially or technically to deploy, and<br>
>> > cause real damage.<br>
>> > I think we need to work hard on mitigating this:<br>
>> > - the feature needs careful design and extra scrutiny, like<br>
>> > security-critical code<br>
>> > - it should be clearly and temporarily marked as experimental, with opt-in<br>
>> > required<br>
>> > - it should be clearly and permanently marked as "makes assumptions about<br>
>> > coding style", with opt-in required.<br>
>> > - bugs need to be thoughtfully addressed<br>
>> > From what I can see MyDeveloperDay is serious about doing all of this.<br>
>> ><br>
>> ><br>
>> > I am, I also think that we shouldn't plough on with individual changes if we see<br>
>> > them as potentially ambiguous, I would rather ignore a change if in doubt, I<br>
>> > don't feel such features need to be 100% catch all (like how sometimes clang<br>
>> > doesn't always tell you about all missing overrides, just as it can rationalize<br>
>> > them), This may require more specific options to ensure we know what an<br>
>> > tok::identifier actually is in order to avoid ambiguities caused by macros (a<br>
>> > little like StatementMacros)<br>
>> ><br>
>> ><br>
>> > *Risk*: clang-format will be overtaken by the complexity of such features,<br>
>> > which will outweigh the benefits (if few use them), hurting maintenance,<br>
>> > causing bugs etc.<br>
>> > However this isn't different from other optional features. Editing tokens<br>
>> > tends to be done as a separate pass which is relatively easy to isolate<br>
>> > (compared to something like supporting a new language). With complexity<br>
>> > isolated, this is mostly just about how maintainers prioritize their<br>
>> > time/attention, which must be left up to them.<br>
>> ><br>
>> ><br>
>> > To be honest these are likely some of the less invasive features (in comparison<br>
>> > to say adding something like adding Whitesmiths style or C#), as you say the<br>
>> > "Passes" give us an easy mechanisms to handle the "OptIn" without adding "if<br>
>> > (...) everywhere and the passes also tend to be very self contained especially<br>
>> > as the Formatting itself is just a Pass in its own right which is performed later.<br>
>> ><br>
>> > I have no concerns over the maintenance other than ensuring we understand how<br>
>> > new passes actually work, but the compartmentalization feels on a par to<br>
>> > compartmentalization of individual clang-tidy checks.<br>
>> ><br>
>> ><br>
>> > Regarding include-ordering: I think this is a valuable feature if you follow<br>
>> > a coding style that allows it to be correct, and it fits well in<br>
>> > clang-format's brand. However it wasn't clearly labeled to emphasize its<br>
>> > caveats, and in hindsight it shouldn't have been made part of the Google<br>
>> > style without further opt-in required.<br>
>> ><br>
>> ><br>
>> > To be honest as a developer I like the brutality of include-ordering, breaking<br>
>> > my code only tells me it isn't robust enough (likely missing forward<br>
>> > declarations or not including what its using)<br>
>> ><br>
>> > The handling of defaults is always difficult as some people want things and<br>
>> > others don't, (hence the need for the RFC), but I've always been clear this<br>
>> > needs to be "Opt-In" from the start. For the majority of developers I would<br>
>> > expect them to continue to use clang-format as a code formatter and nothing<br>
>> > else, but having a ability to make some (not all) obvious changes could<br>
>> > potentially be a great help to improving code<br>
>> ><br>
>> > For example how many times do you see in LLVM the review comment that says<br>
>> > "elide the braces" for<br>
>> ><br>
>> > if (x) {<br>
>> > return;<br>
>> > }<br>
>> ><br>
>> > I feel this is something that clang-format could be made to easily handle. This<br>
>> > RFC is about gaining a general consensus to let us try. We feel we can add even<br>
>> > more value.<br>
>> ><br>
>> > Anyone who knows me, knows I'm very much pro "clang-format all the things"<br>
>> ><br>
>> > MyDeveloperDay<br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > cfe-dev mailing list<br>
>> > <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
>> > <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
>> ><br>
>><br>
>> _______________________________________________<br>
>> cfe-dev mailing list<br>
>> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
>> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
><br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
</blockquote></div></div>