Thank you very much, Zachary :) From: Zachary Turner [mailto:ztur...@google.com] Sent: Monday, December 11, 2017 4:21 PM To: Zhen Cao <zhen....@autodesk.com> Cc: Benoit Belley <benoit.bel...@autodesk.com>; reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org; r...@google.com; cfe-commits@lists.llvm.org Subject: Re: [PATCH] D41081: Fix clang Lexer Windows line-ending bug
I'll let rnk@ weigh in (he's on vacation today though). 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. :) On Mon, Dec 11, 2017 at 1:13 PM Zhen Cao <zhen....@autodesk.com<mailto:zhen....@autodesk.com>> wrote: 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. From: Zachary Turner [mailto:ztur...@google.com<mailto:ztur...@google.com>] Sent: Monday, December 11, 2017 3:42 PM To: Benoit Belley <benoit.bel...@autodesk.com<mailto:benoit.bel...@autodesk.com>> Cc: reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org<mailto:reviews%2bd41081%2bpublic%2b5a71b504a12c1...@reviews.llvm.org>; Zhen Cao <zhen....@autodesk.com<mailto:zhen....@autodesk.com>>; r...@google.com<mailto:r...@google.com> Subject: Re: [PATCH] D41081: Fix clang Lexer Windows line-ending bug 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. 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. On Mon, Dec 11, 2017 at 12:32 PM Benoit Belley <benoit.bel...@autodesk.com<mailto:benoit.bel...@autodesk.com>> wrote: Hi Zachary, 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. In summary, the proposed solution is: * SCM agnostic * Tests the handling both types of line endings on any platforms where the tests are run. Cheers, Benoit From: Zachary Turner <ztur...@google.com<mailto:ztur...@google.com>> Date: lundi 11 décembre 2017 à 15:22 To: "reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org<mailto:reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org>" <reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org<mailto:reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org>> Cc: Zhen Cao <zhen....@autodesk.com<mailto:zhen....@autodesk.com>>, "r...@google.com<mailto:r...@google.com>" <r...@google.com<mailto:r...@google.com>>, Benoit Belley <benoit.bel...@autodesk.com<mailto:benoit.bel...@autodesk.com>> Subject: Re: [PATCH] D41081: Fix clang Lexer Windows line-ending bug 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. Can we just check in two different input files, one with CR and another with CRLF, and use those as inputs to the test? On Mon, Dec 11, 2017 at 12:18 PM Zhen Cao via Phabricator <revi...@reviews.llvm.org<mailto:revi...@reviews.llvm.org>> wrote: caoz added a subscriber: cfe-commits. https://reviews.llvm.org/D41081<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=>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
