[PATCH] D41081: Fix clang Lexer Windows line-ending bug

[Zachary Turner via cfe-commits]

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.cao at 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:zturner at google.com]
> *Sent:* Monday, December 11, 2017 3:42 PM
> *To:* Benoit Belley <Benoit.Belley at autodesk.com>
> *Cc:* reviews+D41081+public+5a71b504a12c16ac at reviews.llvm.org; Zhen Cao <
> zhen.cao at autodesk.com>; rnk at 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.Belley at 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 <zturner at google.com>
> *Date: *lundi 11 décembre 2017 à 15:22
> *To: *"reviews+D41081+public+5a71b504a12c16ac at reviews.llvm.org" <
> reviews+D41081+public+5a71b504a12c16ac at reviews.llvm.org>
> *Cc: *Zhen Cao <zhen.cao at autodesk.com>, "rnk at google.com" <rnk at google.com>,
> Benoit Belley <benoit.belley at 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 <
> reviews at 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=>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171211/f26def91/attachment-0001.html>