<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal">Agree with Dave – I generally find that style issues get in the way of reviewing for correctness/efficiency/whatever. I’d much rather have the author clean up the style first, so reviewers don’t have to “claw through” either the style
comments or the style issues themselves.<o:p></o:p></p>
<p class="MsoNormal">--paulr<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> llvm-dev <llvm-dev-bounces@lists.llvm.org> <b>On Behalf Of
</b>David Blaikie via llvm-dev<br>
<b>Sent:</b> Monday, April 19, 2021 1:19 PM<br>
<b>To:</b> Chris Tetreault <ctetreau@quicinc.com><br>
<b>Cc:</b> llvm-dev@lists.llvm.org; Maxim Kazantsev <mkazantsev@azul.com><br>
<b>Subject:</b> Re: [llvm-dev] clang-tidy makes review a pain<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">Generally, if clang-tidy feedback is correct/actionable I think it's fine - if it's muddying up the review, then asking the author to address the clang-tidy feedback first, before you review the rest seems like a good approach to me.<br>
<br>
On Mon, Apr 19, 2021 at 9:31 AM Chris Tetreault via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<o:p></o:p></p>
</div>
<div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">I agree that the clang-tidy reporting on your linked patch is quite egregious. I think that clang-tidy should only post feedback out of line. Ideally, it should only complain about
actual violations of the llvm coding standards. I see nothing in the coding standards about requiring or preferring that stack locals being declared const if they can be.<o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><br>
If that were the policy, I'd object to it (top level const certainly isn't done pervasively in LLVM and I'd find it "quirky" (& I do push back on it where it's not already in use as a micro-convention in some file/area of the code)), but the policy clang-tidy
seems to be enforcing is to use "const" on "auto&" and "auto*" where possible, which while not quite written in the LLVM style guide, I think has been discussed/generally encouraged in review - the style guide sort of alludes to it in the example here: <a href="https://urldefense.com/v3/__https:/llvm.org/docs/CodingStandards.html*beware-unnecessary-copies-with-auto__;Iw!!JmoZiZGBv3RvKRSx!okooOFf_35XyC-FCnLejKhdv7YRL5KmajydJIvKhGtpysTtDKHbzqLzZpWrS75Yz6Q$">https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto</a> <br>
<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Thanks,<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> Christopher Tetreault<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><b>From:</b> llvm-dev <<a href="mailto:llvm-dev-bounces@lists.llvm.org" target="_blank">llvm-dev-bounces@lists.llvm.org</a>>
<b>On Behalf Of </b>Maxim Kazantsev via llvm-dev<br>
<b>Sent:</b> Monday, April 19, 2021 2:43 AM<br>
<b>To:</b> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<b>Subject:</b> [EXT] [llvm-dev] clang-tidy makes review a pain<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Hello everyone,<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">I started noticing that lately we’ve improved reporting from clang-tidy, pointing out at various formatting issues. However the more verbose it becomes, the more annoyed I feel
about it. For example here:<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><a href="https://urldefense.com/v3/__https:/reviews.llvm.org/D100721__;!!JmoZiZGBv3RvKRSx!okooOFf_35XyC-FCnLejKhdv7YRL5KmajydJIvKhGtpysTtDKHbzqLzZpWqdst-Kaw$" target="_blank">https://reviews.llvm.org/D100721</a><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">It complains literally about every second line, inserting its comments straight into review. They take as much space as the actual code. Maybe it’s just me, but it’s really hard
to me to understand what the patch is actually doing with so many inlined auto-generated comments. Maybe there is a button to hide them somewhere, but I failed to find it.<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">I understand what was the intention, and clang-tidy is a cool thing in general, but it’s getting too intrusive. Does anyone else have the same problem as I do? If there’s a lot
of people whom it annoys, maybe we should think how to make it less invasive. Maybe it should put these comment when the patch gets approved, or something like this.<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Thanks,<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Max<o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
</div>
<p class="MsoNormal">_______________________________________________<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://urldefense.com/v3/__https:/lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev__;!!JmoZiZGBv3RvKRSx!okooOFf_35XyC-FCnLejKhdv7YRL5KmajydJIvKhGtpysTtDKHbzqLzZpWr8A83VZA$" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</div>
</body>
</html>