<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; color: rgb(0, 0, 0); font-size: 16px; font-family: Calibri, sans-serif;">
<div>
<div>
<div>Thanks Zachary.</div>
</div>
</div>
<div><br>
</div>
<div>Maybe, we can approach the issue through another, hopefully less controversial angle. Clang currently has a limited number of tests verifying that it correctly handles all types of line endings. IMHO, Zhen's solution (dos2unix & unix2dos conversions)
provides an handy mechanism to expand these tests without having to duplicate test cases.</div>
<div><br>
</div>
<div>
<div>For reference, here's the current list of clang test files which seems to explicitly test various types of line endings (CRLF instead of LF only):</div>
</div>
<div><br>
</div>
<div>
<blockquote style="margin:0 0 0 40px; border:none; padding:0px;">
<div>clang/test/CXX/lex/lex.literal/lex.string/p4.cpp</div>
<div>clang/test/FixIt/fixit-newline-style.c</div>
<div>clang/test/Frontend/system-header-line-directive-ms-lineendings.c</div>
<div><br>
</div>
</blockquote>
<div>
<div>And here's the list of remaining clang test files with CRLF line endings, although AFAICT, that seems to be unintentional:</div>
</div>
<div><br>
</div>
<blockquote style="margin:0 0 0 40px; border:none; padding:0px;">
<div>clang/test/CXX/expr/expr.prim/expr.prim.lambda/p15-star-this-capture.cpp</div>
<div>clang/test/CodeGenCXX/attr-x86-no_caller_saved_registers.cpp</div>
<div>clang/test/CodeGenOpenCL/no-signed-zeros.cl</div>
<div>clang/test/Driver/ps4-analyzer-defaults.cpp</div>
<div>clang/test/Frontend/Inputs/rewrite-includes-bom.h</div>
<div>clang/test/Index/preamble-reparse-with-BOM.m</div>
<div>clang/test/Misc/ast-dump-c-attr.c</div>
<div>clang/test/OpenMP/simd_codegen.cpp</div>
<div>clang/test/Preprocessor/macro_vaopt_check.cpp</div>
<div>clang/test/Preprocessor/macro_vaopt_expand.cpp</div>
<div>clang/test/SemaCXX/attr-non-x86-no_caller_saved_registers.cpp</div>
<div>clang/test/SemaCXX/attr-x86-no_caller_saved_registers.cpp</div>
<div>clang/test/SemaOpenCL/array-init.cl</div>
</blockquote>
</div>
<blockquote style="margin:0 0 0 40px; border:none; padding:0px;">
<div><br>
</div>
</blockquote>
<div>I am of course opened to other solutions to improve the current test coverage.</div>
<div><br>
</div>
<div>Cheers,</div>
<div>Benoit</div>
<div><br>
</div>
<div><br>
</div>
<span id="OLK_SRC_BODY_SECTION">
<div style="font-family:Calibri; font-size:11pt; text-align:left; color:black; BORDER-BOTTOM: medium none; BORDER-LEFT: medium none; PADDING-BOTTOM: 0in; PADDING-LEFT: 0in; PADDING-RIGHT: 0in; BORDER-TOP: #b5c4df 1pt solid; BORDER-RIGHT: medium none; PADDING-TOP: 3pt">
<span style="font-weight:bold">From: </span>Zhen Cao <<a href="mailto:zhen.cao@autodesk.com">zhen.cao@autodesk.com</a>><br>
<span style="font-weight:bold">Date: </span>lundi 11 décembre 2017 à 16:23<br>
<span style="font-weight:bold">To: </span>Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>><br>
<span style="font-weight:bold">Cc: </span>Benoit Belley <<a href="mailto:benoit.belley@autodesk.com">benoit.belley@autodesk.com</a>>, "<a href="mailto:reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org">reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org</a>"
<<a href="mailto:reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org">reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org</a>>, "<a href="mailto:rnk@google.com">rnk@google.com</a>" <<a href="mailto:rnk@google.com">rnk@google.com</a>>, "<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>"
<<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>><br>
<span style="font-weight:bold">Subject: </span>RE: [PATCH] D41081: Fix clang Lexer Windows line-ending bug<br>
</div>
<div><br>
</div>
<blockquote id="MAC_OUTLOOK_ATTRIBUTION_BLOCKQUOTE" style="BORDER-LEFT: #b5c4df 5 solid; PADDING:0 0 0 5; MARGIN:0 0 0 5;">
<div 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">
<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;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
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;}
/* List Definitions */
@list l0
{mso-list-id:2039965533;
mso-list-template-ids:1563078834;}
@list l0:level1
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level2
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:1.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level3
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:1.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level4
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:2.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level5
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:2.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level6
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:3.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level7
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:3.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level8
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:4.0in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
@list l0:level9
{mso-level-number-format:bullet;
mso-level-text:?;
mso-level-tab-stop:4.5in;
mso-level-number-position:left;
text-indent:-.25in;
mso-ansi-font-size:10.0pt;
font-family:Symbol;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></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]-->
<div lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">Thank you very much, Zachary :)<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><b>From:</b> Zachary Turner [<a href="mailto:zturner@google.com">mailto:zturner@google.com</a>]
<br>
<b>Sent:</b> Monday, December 11, 2017 4:21 PM<br>
<b>To:</b> Zhen Cao <<a href="mailto:zhen.cao@autodesk.com">zhen.cao@autodesk.com</a>><br>
<b>Cc:</b> Benoit Belley <<a href="mailto:Benoit.Belley@autodesk.com">Benoit.Belley@autodesk.com</a>>;
<a href="mailto:reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org">reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org</a>;
<a href="mailto:rnk@google.com">rnk@google.com</a>; <a href="mailto:cfe-commits@lists.llvm.org">
cfe-commits@lists.llvm.org</a><br>
<b>Subject:</b> Re: [PATCH] D41081: Fix clang Lexer Windows line-ending bug<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">I'll let rnk@ weigh in (he's on vacation today though).<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I don't feel comfortable lgtm'ing any change where "don't use core.autocrlf=true" is an alternative solution, but if other people want to disagree with me and lgtm this, then that suggests I'm in the minority, which is fine. :)<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Mon, Dec 11, 2017 at 1:13 PM Zhen Cao <<a href="mailto:zhen.cao@autodesk.com">zhen.cao@autodesk.com</a>> wrote:<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">I think adding two lit substitutions is not a significant issue since dos2unix and unix2dos are common Unix utilities. This solution also has the advantage of testing both styles
of line-endings with only one test case, which I think actually reduces complexity and maintenance burden.<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"><b>From:</b> Zachary Turner [mailto:<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>]
<br>
<b>Sent:</b> Monday, December 11, 2017 3:42 PM<br>
<b>To:</b> Benoit Belley <<a href="mailto:Benoit.Belley@autodesk.com" target="_blank">Benoit.Belley@autodesk.com</a>><br>
<b>Cc:</b> <a href="mailto:reviews%2BD41081%2Bpublic%2B5a71b504a12c16ac@reviews.llvm.org" target="_blank">
reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org</a>; Zhen Cao <<a href="mailto:zhen.cao@autodesk.com" target="_blank">zhen.cao@autodesk.com</a>>;
<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a><o:p></o:p></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><br>
<b>Subject:</b> Re: [PATCH] D41081: Fix clang Lexer Windows line-ending bug<o:p></o:p></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">To be honest, I'm not a fan of testing code in a way that tries to work around issues related to source control. I think we had a similar discussion before on a previous patch,
and my suggested fix was to simply set core.autocrlf=false in your git config. This solves all of these problems and does not come with any real downsides that I'm aware of.<o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">It also yields better tests IMO, because the fewer steps of translation that an input has to go through before being fed to the test program the better. If you want to test the
behavior of a file with \r\n, just check in an input file with \r\n and you're ready to go. Furthermore, by not doing this we are adding complexity (and hence, technical debt) *to the code* to deal with issues that could easily be solved using an appropriate
developer configuration. I'm not saying that we should not be testing the case of \r\n newlines, because we can't control what perforce outputs and the tool itself needs to work with either style of newline. But we can control how we configure our git clones,
and if doing so leads to better tests with less testing infrastructure hacks and workarounds, then I think we should do that.<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>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">On Mon, Dec 11, 2017 at 12:32 PM Benoit Belley <<a href="mailto:Benoit.Belley@autodesk.com" target="_blank">Benoit.Belley@autodesk.com</a>> wrote:<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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<div>
<div>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black">Hi Zachary,</span><o:p></o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black">We are trying to be agnostic to how a particular SCM (SVN, Git) is handling line termination. For example, any CR, CRLF line-ending would
be translated automatically by Git (CR only on unix, and CRLF on windows) unless these files are marked explicitly as binary files using a special .gitattribute file.</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black">In summary, the proposed solution is:</span><o:p></o:p></p>
</div>
<ul type="disc">
<li class="MsoNormal" style="color:black;mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:0in;mso-list:l0 level1 lfo1">
<span style="font-size:12.0pt">SCM agnostic</span><o:p></o:p></li><li class="MsoNormal" style="color:black;mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:0in;mso-list:l0 level1 lfo1">
<span style="font-size:12.0pt">Tests the handling both types of line endings on any platforms where the tests are run.</span><o:p></o:p></li></ul>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black">Cheers,</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black">Benoit</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p>
</div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><b><span style="color:black">From:
</span></b><span style="color:black">Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>><br>
<b>Date: </b>lundi 11 décembre 2017 à 15:22<br>
<b>To: </b>"<a href="mailto:reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org" target="_blank">reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org</a>" <<a href="mailto:reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org" target="_blank">reviews+D41081+public+5a71b504a12c16ac@reviews.llvm.org</a>><br>
<b>Cc: </b>Zhen Cao <<a href="mailto:zhen.cao@autodesk.com" target="_blank">zhen.cao@autodesk.com</a>>, "<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>" <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>>, Benoit Belley
<<a href="mailto:benoit.belley@autodesk.com" target="_blank">benoit.belley@autodesk.com</a>><br>
<b>Subject: </b>Re: [PATCH] D41081: Fix clang Lexer Windows line-ending bug</span><o:p></o:p></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black"> </span><o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #B5C4DF 4.5pt;padding:0in 0in 0in 4.0pt;margin-left:3.75pt;margin-top:5.0pt;margin-right:0in;margin-bottom:5.0pt" id="m_-7756912001741113462m_-7611074244742519728MAC_OUTLOOK_ATTRIBUTION_BLOCKQUOTE">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black">Would it be possible to do this without any substitutions at all? It seems a little heavy handed to add a new lit substitution for something
that is only going to be used in one test.<br>
<br>
Can we just check in two different input files, one with CR and another with CRLF, and use those as inputs to the test?</span><o:p></o:p></p>
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="font-size:12.0pt;color:black">On Mon, Dec 11, 2017 at 12:18 PM Zhen Cao via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>>
wrote:</span><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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<p class="MsoNormal" style="mso-margin-top-alt:auto;margin-bottom:12.0pt"><span style="font-size:12.0pt;color:black">caoz added a subscriber: cfe-commits.<br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_D41081&d=DwMFaQ&c=76Q6Tcqc-t2x0ciWn7KFdCiqt6IQ7a_IF9uzNzd_2pA&r=wR2gM5Rr7Ie8nJT0AKKx0nretMcnu3YZMyPRVEnnIr0&m=uejO8PxG5kINlnu9hNgIioUZcZun7px3wczMnmy_ocU&s=chrRP4fl0LJR04El0VzGUg6oiCuC7dRdQOhbn7m7Jt0&e=" target="_blank">https://reviews.llvm.org/D41081</a><br>
<br>
</span><o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
</span>
</body>
</html>