This is an archive of the discontinued LLVM Phabricator instance.

[clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings
ClosedPublic

Authored by aganea on Aug 21 2019, 1:54 PM.

Diff Detail

Event Timeline

aganea created this revision.Aug 21 2019, 1:54 PM
aganea updated this revision to Diff 216474.Aug 21 2019, 1:56 PM

Use proper test file.

arphaman added inline comments.Aug 22 2019, 3:49 PM
lib/Lex/DependencyDirectivesSourceMinimizer.cpp
213

Nit: Please add && "expected newline") to the assert.

240

This return now changes behavior of skipToNewlineRaw for the clients, as First is now right after the newline if this return is taken, as opposed to right before the newline, like it used to be before this patch. I think you need to move First += Len after this return.

aganea updated this revision to Diff 217055.Aug 25 2019, 11:17 AM
aganea marked 2 inline comments as done.

As requested.

aganea updated this revision to Diff 217179.Aug 26 2019, 9:34 AM

Fixed unit tests.

This revision is now accepted and ready to land.Aug 26 2019, 10:14 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 4:19 PM
aganea updated this revision to Diff 217292.Aug 26 2019, 6:24 PM
aganea added a reviewer: rsmith.

This failed the build - I've changed unix2dos to svn:eol-style CRLF instead.

aganea reopened this revision.Aug 26 2019, 6:24 PM
This revision is now accepted and ready to land.Aug 26 2019, 6:24 PM
aganea requested review of this revision.Aug 26 2019, 6:24 PM

Will the git monorepo handle svn:eol-style correctly?

This failed the build - I've changed unix2dos to svn:eol-style CRLF instead.

Can we count on tr when we have shell? If so, I suggest this instead:

// REQUIRES: shell
// RUN: tr '\n' '\r\n' <%s | %clang_cc1 -DOTHER -print-dependency-directives-minimized-source 2>&1 | FileCheck %s

This failed the build - I've changed unix2dos to svn:eol-style CRLF instead.

Can we count on tr when we have shell? If so, I suggest this instead:

// REQUIRES: shell
// RUN: tr '\n' '\r\n' <%s | %clang_cc1 -DOTHER -print-dependency-directives-minimized-source 2>&1 | FileCheck %s

Eh, nevermind, tr doesn't handle multi-character replacements. Better to answer Alex's question.

aganea updated this revision to Diff 217527.Aug 27 2019, 4:31 PM
aganea added subscribers: jyknight, rnk.

Will the git monorepo handle svn:eol-style correctly?

Other files in the repo already use svn:eol-style. I'm not sure what happens with SVN properties when commits are replicated in git. @jyknight @rnk ? I've added .gitattributes to ensure these files are checked out from git with the proper line ending.

rnk added a comment.Aug 27 2019, 4:44 PM

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 27 2019, 5:02 PM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Aug 28 2019, 2:12 AM
In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This broke users of the monorepo, where the file would show up as having changed locally, and with no way to reset it. I'm guessing that's because it was checked in with LF endings and then because of the .gitattributes file, it changes at checkout. I think the correct solution would be to check in the file with CRLF endings and not set any attributes or stuff. (Though this is all super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This broke users of the monorepo, where the file would show up as having changed locally, and with no way to reset it. I'm guessing that's because it was checked in with LF endings and then because of the .gitattributes file, it changes at checkout. I think the correct solution would be to check in the file with CRLF endings and not set any attributes or stuff. (Though this is all super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.

It already had CRLF endings locally before commit.
It is strange, the file shows up as having CRLF endings in the old revision in git, before the revert: https://raw.githubusercontent.com/llvm/llvm-project/9774a2ba279aea35f166b8ca489d0e8292026c38/clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
The same kind of problem occured in rL311683, later fixed by rL311732.
If you're fine with that, I'll do the same thing: remove .gitattributes and just re-commit the file minimize_source_to_dependency_directives_invalid_error.c with svn:eol-style CRLF.

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This broke users of the monorepo, where the file would show up as having changed locally, and with no way to reset it. I'm guessing that's because it was checked in with LF endings and then because of the .gitattributes file, it changes at checkout. I think the correct solution would be to check in the file with CRLF endings and not set any attributes or stuff. (Though this is all super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.

Fwiw, this seems to me somewhat like a git bug; if I edit the .gitattributes file and comment out the problematic line, do a git diff, then uncomment the line again, then git diff suddenly shows no changes.

hans added a comment.Aug 28 2019, 6:46 AM
In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This broke users of the monorepo, where the file would show up as having changed locally, and with no way to reset it. I'm guessing that's because it was checked in with LF endings and then because of the .gitattributes file, it changes at checkout. I think the correct solution would be to check in the file with CRLF endings and not set any attributes or stuff. (Though this is all super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.

It already had CRLF endings locally before commit.
It is strange, the file shows up as having CRLF endings in the old revision in git, before the revert: https://raw.githubusercontent.com/llvm/llvm-project/9774a2ba279aea35f166b8ca489d0e8292026c38/clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
The same kind of problem occured in rL311683, later fixed by rL311732.
If you're fine with that, I'll do the same thing: remove .gitattributes and just re-commit the file minimize_source_to_dependency_directives_invalid_error.c with svn:eol-style CRLF.

Yes, I think that should work. (But these things always break in surprising ways.)