Skip to content

Conversation

@nicolasstucki
Copy link
Contributor

No description provided.

@nicolasstucki
Copy link
Contributor Author

@alemannosara here I will add a way for you to create the precise positions inside string literals you need.

@smarter
Copy link
Member

smarter commented Mar 24, 2019

I thought we discussed this and decided to not allow users to define custom positions since they're too easy to get wrong which might lead to crash when they're checked later ?

@nicolasstucki
Copy link
Contributor Author

This PR doesn't actually allow the position to be set on a tree. The only way to use it is in an error message. Though I could add a special error method that receives custom offsets instead.

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM

* @param start index of the start of the range (0 <= start < end)
* @param end index of the end of the range (start < end <= sizeOf(pos.sourceFile))
*/
def Position_withOffset(self: Position)(start: Int, end: Int): Position
Copy link
Contributor

@liufengyun liufengyun Mar 25, 2019

Choose a reason for hiding this comment

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

When I see this API, the first guess is that the offset is relative to the given position self. But in the implementation, only the source file is used. It seems this is better to be defined as a constructor for Position which takes sourceFile directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be terrible, each source position will try to reload the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should have the source file abstraction in TASTy reflect

@nicolasstucki
Copy link
Contributor Author

Replaced by #6158

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.

3 participants