4

Fix handling of carriage return. by chriseth · Pull Request #11071 · ethereum/so...

 3 years ago
source link: https://github.com/ethereum/solidity/pull/11071
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

Copy link

Member

@cameel cameel on Mar 9

This does prevent the crash but I'm not sure it actually fixes the underlying issue.

I think that the problem might actually be in source->lineAtPosition(). It seems that it strips a single \r character from the line which is the reason why start ends up being past the end of the line. For example if the line is \r\r\r\r, it returns \r\r\r. This is inconsistent with source->translatePositionToLineColumn(), which just ignores \r characters completely.

Also, even after the fix the locations shown in error messages seem off. For example for \rabc you get this:

Warning: Yul is still experimental. Please use the output with care.
Error: Expected keyword "object".
 --> /tmp/cr.yul:1:2:
  |
abc
  |  ^^^

Copy link

Contributor

Author

@chriseth chriseth on Mar 9

Yes, that is indeed the underlying issue, but I still think that this is the best fix. The problem with \rabc is a different one.

Copy link

Member

@cameel cameel on Mar 9

Yeah, I think the problem is here:

if (!line.empty() && line.back() == '\r') line.pop_back();

It looks like it's meant to handle the \r left over after stripping \n on Windows. But for this to work correctly translatePositionToLineColumn() should also treat that \r in the same special way - as a part of the line ending - and it does not.

Copy link

Member

@cameel cameel on Mar 9

translatePositionToLineColumn() could be causing similar problems in other places that we just haven't detected yet.

Copy link

Contributor

Author

@chriseth chriseth on Mar 9

I think translatePositionToLineColumn is perfectly implemented according to the specification. It is lineAtPosition that is questionable.

Copy link

Contributor

Author

@chriseth chriseth on Mar 9

We could change lineAtPosition to remove all trailing whitespace....

Copy link

Member

@cameel cameel on Mar 9

Just curious, which specification do you mean? Do we have newline handling documented somewhere?

We could change lineAtPosition to remove all trailing whitespace....

Well, maybe. I wouldn't really expect CharStream to do such a transformation but on the other hand trailing whitespace is mostly meaningless so maybe it's better off that way anyway. I'd at least adjust the name in that case.

Copy link

Contributor

Author

@chriseth chriseth on Mar 9

Ok, no specification, but rather something like "the simplest definition of "line number" that would make sense".


report

Recommend

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK