Skip to content
This repository has been archived by the owner on May 4, 2024. It is now read-only.

support windows \r encoding #18

Closed
wants to merge 1 commit into from

Conversation

LemonHX
Copy link
Contributor

@LemonHX LemonHX commented Apr 14, 2022

support windows benchmarking

Motivation

(Write your motivation for proposed changes here.)

Have you read the Contributing Guidelines on pull requests?

(Write your answer here.)

Test Plan

(Share your test plan here. If you changed code, please provide us with clear instructions for verifying that your changes work.)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/diem/diem/tree/main/developers.diem.com, and link to your PR here.)

If targeting a release branch, please fill the below out as well

  • Justification and breaking nature (who does it affect? validators, full nodes, tooling, operators, AOS, etc.)
  • Comprehensive test results that demonstrate the fix working and not breaking existing workflows.
  • Why we must have it for V1 launch.
  • What workarounds and alternative we have if we do not push the PR.

support windows benchmarking
@LemonHX
Copy link
Contributor Author

LemonHX commented Apr 14, 2022

@wrwg transfered

@tnowacki
Copy link
Member

After discussing it, we will look at a PR for unicode characters in comments, but will not be accepting anything at this time that allows \r, sorry

@wrwg
Copy link
Member

wrwg commented Apr 14, 2022

Just curious: if only the specific Windows sequence \r\n would be accepted, but not a standalone \r, wouldn't that solve the security problem? It appears pretty restrictive to not allow Move on native Windows.

@sblackshear
Copy link
Member

T

Just curious: if only the specific Windows sequence \r\n would be accepted, but not a standalone \r, wouldn't that solve the security problem? It appears pretty restrictive to not allow Move on native Windows.

That solution sounds reasonable to me, but I don't have the right context on the parser to understand how easy it is to do this.

@LemonHX : would you mind de-scoping this PR to the parser change only (i.e., move the benchmarking code to a separate PR?)

@LemonHX
Copy link
Contributor Author

LemonHX commented Apr 15, 2022

@sblackshear I will do that

@LemonHX
Copy link
Contributor Author

LemonHX commented Apr 15, 2022

@wrwg so we could add to our documentation that if you are windows user please config your git before generating any move file

@LemonHX
Copy link
Contributor Author

LemonHX commented Apr 15, 2022

@wrwg \r is used in native mac...

@wrwg
Copy link
Member

wrwg commented Apr 15, 2022

@wrwg \r is used in native mac...

... and we will not allow it if it is used. We only allow regular ascii files, where lines can be separated either with \n (mac) or \r\n (windows). I think this is very easy to add to the lexer.

@wrwg
Copy link
Member

wrwg commented May 13, 2022

Superseded by #90

@wrwg wrwg closed this May 13, 2022
jiangying000 pushed a commit to jiangying000/move that referenced this pull request Sep 27, 2022
…eset

[move-vm] allow gas metering to be customized
brson added a commit to brson/move that referenced this pull request Jan 18, 2023
Introduce Move source to LLVM IR tests
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants