<div dir="ltr">I'll let rnk@ weigh in (he's on vacation today though).<div><br></div><div>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. :)</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Dec 11, 2017 at 1:13 PM Zhen Cao <<a href="mailto:zhen.cao@autodesk.com">zhen.cao@autodesk.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div class="m_-7756912001741113462WordSection1">
<p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><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></p></div></div><div lang="EN-US" link="blue" vlink="purple"><div class="m_-7756912001741113462WordSection1"><p class="MsoNormal"><br>
<b>Subject:</b> Re: [PATCH] D41081: Fix clang Lexer Windows line-ending bug<u></u><u></u></p></div></div><div lang="EN-US" link="blue" vlink="purple"><div class="m_-7756912001741113462WordSection1">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
</div>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">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:<u></u><u></u></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>
<div>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black">Hi Zachary,<u></u><u></u></span></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><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.<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black">In summary, the proposed solution is:<u></u><u></u></span></p>
</div>
<ul type="disc">
<li class="MsoNormal" style="color:black;margin-left:0in">
<span style="font-size:12.0pt">SCM agnostic<u></u><u></u></span></li><li class="MsoNormal" style="color:black;margin-left:0in">
<span style="font-size:12.0pt">Tests the handling both types of line endings on any platforms where the tests are run.<u></u><u></u></span></li></ul>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black">Cheers,<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black">Benoit<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black"><u></u> <u></u></span></p>
</div>
<div style="border:none;border-top:solid #b5c4df 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><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<u></u><u></u></span></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:12.0pt;color:black"><u></u> <u></u></span></p>
</div>
<blockquote style="border:none;border-left:solid #b5c4df 4.5pt;padding:0in 0in 0in 4.0pt;margin-left:3.75pt;margin-right:0in" id="m_-7756912001741113462m_-7611074244742519728MAC_OUTLOOK_ATTRIBUTION_BLOCKQUOTE">
<div>
<div>
<p class="MsoNormal"><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?<u></u><u></u></span></p>
<div>
<div>
<p class="MsoNormal"><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:<u></u><u></u></span></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">
<p class="MsoNormal" style="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>
<br>
<u></u><u></u></span></p>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div></div></blockquote></div>