Skip to content

Conversation

@agniuks
Copy link
Contributor

@agniuks agniuks commented Apr 27, 2021

Adds two forked folly files in order to re-enable C4244 and C4267 warning suppressions. I opened a PR in folly to address this from there, and will add an issue to track updating folly and removing these files.
Closes #6918.

Microsoft Reviewers: Open in CodeFlow

@agniuks agniuks requested a review from a team as a code owner April 27, 2021 00:19
@ghost ghost added the Area: Infrastructure label Apr 27, 2021
Copy link
Member

@asklar asklar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some questions.

Meta point (this one shouldn't block this PR), it might be better to use patches instead of forked files to be able to PR the individual forked change rather than an entire file that has maybe one line different. Thoughts?

@ghost ghost added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Apr 27, 2021
@agniuks
Copy link
Contributor Author

agniuks commented Apr 27, 2021

Left some questions.

Meta point (this one shouldn't block this PR), it might be better to use patches instead of forked files to be able to PR the individual forked change rather than an entire file that has maybe one line different. Thoughts?

@asklar That sounds like the better approach, especially with the minimal changes. I can update this to use patches instead.

edit: Will push this to get the changes in now and will follow up on potentially changing this to patch files separately.

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@asklar
Copy link
Member

asklar commented Apr 29, 2021

[talked offline, we still need to figure out how x86 should/will work]

@agniuks
Copy link
Contributor Author

agniuks commented Apr 29, 2021

@asklar, I updated to_ascii_port_clzll() method to call builtin_clzll() on x86. builtin_clzll() will then call __builtin_clz() which is supported only in VS 16.3-16.8 but code added in folly handles the logic for calling polyfill that uses _BitScanReverse when __builtin_clz is not available

With the forked files it's hard to see the diffs; for reference the only changes I've made to these files from what we were using before are in ToAscii.h lines 59-63(adding check for x86), 64/69(updates to match changes upstream - return type changed to auto from int and added return statement in line 69), 84(added cast), & 233(added cast). in Dynamic.cpp the only change is an added cast on line 300.

@agniuks agniuks merged commit 29481ff into microsoft:master Apr 30, 2021
@agniuks agniuks deleted the forkedfolly branch April 30, 2021 15:40
@agniuks agniuks mentioned this pull request May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SDL compliance

2 participants