Skip to content

Conversation

@DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Sep 21, 2020

For static constant expressions like the following, add an extra range for the entire StaticConstantExpr including 'const'.

MyTypeProvider<const(...)>

This will make it easier to understand the boundaries of the entire static constant expression

Copy link
Member

@auduchinok auduchinok Sep 21, 2020

Choose a reason for hiding this comment

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

Is there a need for this field? Let's calculate this range from expr, like it's done in SynExpr.Do, SynExpr.Lazy, and SynExpr.Assert?

@DedSec256 DedSec256 force-pushed the ber.a/staticConstExprRange branch from 1f33cb4 to 62c9f5e Compare September 21, 2020 11:25
Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Your previous change with the exprRange to me feels more correct because if we ever need to get the range that doesn't include const, we would be able to. But, it would be inconsistent with other SynExprs based on what @auduchinok mentioned. Therefore, I think the current change is fine.

@cartermp cartermp merged commit bec9b08 into dotnet:main Sep 21, 2020
@auduchinok
Copy link
Member

auduchinok commented Sep 21, 2020

@TIHan it's still possible to get the expression range, it'll be calculated from the expression itself instead of being stored in its parent node. Since it's not something used too often it looks like a good trade off and apparently it hasn't been an issue with the other cases mentioned above either.

@TIHan
Copy link
Contributor

TIHan commented Sep 22, 2020

@auduchinok That works better actually; I didn't initially think about that.

@DedSec256 DedSec256 deleted the ber.a/staticConstExprRange branch February 8, 2021 10:25
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants