Skip to content

CSHARP-5563: Create AstExpression extension methods to make testing for constants easier. #1668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rstam
Copy link
Contributor

@rstam rstam commented Apr 11, 2025

Here's the pattern:

public static bool IsInt32Constant(this AstExpression expression)
public static bool IsInt32Constant(this AstExpression expression, int value)
public static bool IsInt32Constant(this AstExpression expression, out int value)

The first tests whether it is an integer constant (the value doesn't matter)
The second test whether it is an integer constant (of a particular value)
The third tests whether is it an integer constant (and returns the value in the out parameter)

@rstam rstam requested review from sanych-sun and adelinowona April 11, 2025 17:56
@rstam rstam requested a review from a team as a code owner April 11, 2025 17:56

public static bool IsMaxInt32(this AstExpression expression)
=> expression.IsInt32Constant(out var value) && value == int.MaxValue;

public static bool IsRootVar(this AstExpression expression)
=> expression is AstVarExpression varExpression && varExpression.Name == "ROOT" && varExpression.IsCurrent;

public static bool IsStringConstant(this AstExpression expression, string value)
Copy link
Member

@sanych-sun sanych-sun Apr 15, 2025

Choose a reason for hiding this comment

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

All such IsXXXConstant methods could be simplified by calling appropriate IsXXXConstant with out parameter. For example:

public static bool IsStringConstant(this AstExpression expression, string value)
  => expression.IsStringConstant(out var val) && val == value

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 is true.

My thinking was that this was slightly more efficient than adding one more level of indirection with the additional bool return value and copying of out parameters.

I'm willing to make this change if you confirm that you still want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree that your suggestion is short and concise and elegant.

The only possible reason for not doing it would be performance, and any concerns about performance in this method might be premature optimization anyway.

Copy link
Member

Choose a reason for hiding this comment

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

If we worried about the implications of calling the method instead of the executing the code itself, we can force compiler to inline the code whenever it's possible by marking the calling method with:
[MethodImplAttribute(MethodImplOptions.AggressiveInlining)}

@rstam rstam requested a review from sanych-sun April 17, 2025 13:53

public static bool IsMaxInt32(this AstExpression expression)
=> expression.IsInt32Constant(out var value) && value == int.MaxValue;

public static bool IsRootVar(this AstExpression expression)
=> expression is AstVarExpression varExpression && varExpression.Name == "ROOT" && varExpression.IsCurrent;

public static bool IsStringConstant(this AstExpression expression, string value)
Copy link
Member

Choose a reason for hiding this comment

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

If we worried about the implications of calling the method instead of the executing the code itself, we can force compiler to inline the code whenever it's possible by marking the calling method with:
[MethodImplAttribute(MethodImplOptions.AggressiveInlining)}

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.

2 participants