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> 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] > *Sent:* Monday, December 11, 2017 3:42 PM > *To:* Benoit Belley <benoit.bel...@autodesk.com> > *Cc:* reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org; Zhen Cao < > zhen....@autodesk.com>; 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> > 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> > *Date: *lundi 11 décembre 2017 à 15:22 > *To: *"reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org" < > reviews+d41081+public+5a71b504a12c1...@reviews.llvm.org> > *Cc: *Zhen Cao <zhen....@autodesk.com>, "r...@google.com" <r...@google.com>, > Benoit Belley <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> 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
