Merge unreleased/rs3 to release/1.7 #4195
Conversation
| // * Does not add anything to the file description | ||
|
|
||
| // ChakraCore RELEASE and PRERELEASE flags | ||
| #define CHAKRA_CORE_VERSION_RELEASE 1 |
There was a problem hiding this comment.
Why did you change this? My understanding is that it is set to 1 since 1.7 is a release branch?
There was a problem hiding this comment.
You are correct, this was an oversight.
|
Commenting for the record that the size of this diff is mostly line ending normalizations to test files: With line ending changes: Without line ending changes: |
| *.h text eol=lf diff=cpp | ||
| *.inl text eol=lf diff=cpp | ||
| *.vcproj text eol=crlf diff=xml | ||
| *.vcxproj text eol=crlf diff=xml |
There was a problem hiding this comment.
so far, AFAIK, we've been treating these files as LF with no ill effects.
There was a problem hiding this comment.
+1, where did this change originate from?
There was a problem hiding this comment.
Sounds like @Penguinwizzard recommended this spec as a way to help git deal with merge conflicts on line endings -- and it possibly had the opposite from the intended effect. :P
There was a problem hiding this comment.
There's an easy way to strip line ending changes from a PR. I'll have to dig it up from email somewhere.
| *.inl text eol=lf diff=cpp | ||
| *.vcproj text eol=crlf diff=xml | ||
| *.vcxproj text eol=crlf diff=xml | ||
| *.sln text eol=crlf diff=xml |
There was a problem hiding this comment.
*.sln text eol=crlf [](start = 0, length = 19)
yes, AFAIK, sln requires CRLF
lib/Backend/LowerMDShared.cpp
Outdated
| return labelDone; | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: added unnecessary newline
| // NOTE: If there is a merge conflict the correct fix is to make a new GUID. | ||
|
|
||
| // {C6C1CBF8-B232-4E4C-8681-34E404F78F16} | ||
| // {F856871A-E536-4CA1-83E6-819E26C7DC99} |
There was a problem hiding this comment.
Just a note (in case it wasn't already done this way) that if there is a merge conflict in this file, please generate a new GUID.
There was a problem hiding this comment.
This is a new GUID. 👍
| } | ||
| #endif | ||
| #endif | ||
|
|
There was a problem hiding this comment.
nit: unnecessary extra line
| #endif | ||
| }; | ||
| #endif | ||
|
|
There was a problem hiding this comment.
nit: unnecessary extra line
|
LGTM but I question the need to take these line ending changes in the test files. I'd say we re-normalize them to what they were before unless there's a good reason to change. /cc @tcare |
|
The issue with the test files is that they're currently specified as "-crlf"; this doesn't mean "not crlf", but means "-text", and as such disables any git normalization and has them explicitly use the user's platform's eol normalization. We should likely have them set as "text eol=lf". |
|
Changing line endings should not be a part of this PR. Let's address this during downtime in December along with other invasive .gitattributes changes I want to make. |
|
Do we have a list of changes for this PR? |
|
@Penguinwizzard as always, I'll add that because some of our (especially parser-related) JS tests are sensitive to line endings and other whitespace, a blanket policy for line endings in JS files might be ill-advised. However, OTOH I'm not sure why we never bothered to move all such files to their own directory and make a specific policy for that. |
|
@dilijev is right, normalizing line endings in .js files will introduce false negatives for scanner tests. |
4b47044 to
036fe50
Compare
Merge pull request #4195 from boingoing:mergeback_rs3_17
The reason everyone is skittish about improving our test code is we don't when a change might cause the test to stop covering the intended code path. Too bad we don't have a way to determine that a test is testing the right things, and to protect against test coverage regression. (Code coverage isn't quite the same thing, but would be a start.) |
No description provided.