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

[Zhen Cao via cfe-commits]

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<mailto: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<mailto:zturner at google.com>>
Date: lundi 11 décembre 2017 à 15:22
To: "reviews+D41081+public+5a71b504a12c16ac at reviews.llvm.org<mailto:reviews+D41081+public+5a71b504a12c16ac at reviews.llvm.org>" <reviews+D41081+public+5a71b504a12c16ac at reviews.llvm.org<mailto:reviews+D41081+public+5a71b504a12c16ac at reviews.llvm.org>>
Cc: Zhen Cao <zhen.cao at autodesk.com<mailto:zhen.cao at autodesk.com>>, "rnk at google.com<mailto:rnk at google.com>" <rnk at google.com<mailto:rnk at google.com>>, Benoit Belley <benoit.belley at autodesk.com<mailto: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<mailto: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/2e940594/attachment.html>