Skip to content

Extract constant value of enum member equal clauses#4309

Merged
hvitved merged 4 commits intogithub:mainfrom
tamasvajk:feature/enum-value-init
Sep 28, 2020
Merged

Extract constant value of enum member equal clauses#4309
hvitved merged 4 commits intogithub:mainfrom
tamasvajk:feature/enum-value-init

Conversation

@tamasvajk
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the C# label Sep 21, 2020
? Expression.ValueAsString(constValue.Value)
: null;

var expr = new Expression(new ExpressionInfo(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why aren't we extracting the expression itself as a subexpression?

enum MyEnum
{
  A = 1+2+3,
  B
}

If I read this correctly, then 1+2+3 is extracted here as a field access instead of an addition.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, we could extract that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably doesn't matter too much. One case that might be interesting if the constant expression contains a reference to a constant and in some check we need all the references to that constant.

@tamasvajk tamasvajk force-pushed the feature/enum-value-init branch 3 times, most recently from 3d89fe9 to d862725 Compare September 22, 2020 06:50
@tamasvajk tamasvajk marked this pull request as ready for review September 22, 2020 06:51
@tamasvajk tamasvajk requested a review from a team as a code owner September 22, 2020 06:51
hvitved
hvitved previously approved these changes Sep 22, 2020
# 40| 1: [MemberConstantAccess] access to constant AnotherBlue
# 40| 1: [AssignExpr] ... = ...
# 40| 0: [AddExpr] ... + ...
# 40| 0: [CastExpr] (...) ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hvitved These casts look a bit strange. How come we have no TypeAccess below these nodes?

@tamasvajk tamasvajk force-pushed the feature/enum-value-init branch from b36b213 to f17eee0 Compare September 22, 2020 21:12
@tamasvajk tamasvajk marked this pull request as draft September 23, 2020 08:14
@tamasvajk tamasvajk marked this pull request as ready for review September 24, 2020 06:08
@tamasvajk tamasvajk requested a review from a team September 24, 2020 06:09
hvitved
hvitved previously approved these changes Sep 24, 2020
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM. Let's run a CSharp-Differences job before we merge though.

@tamasvajk
Copy link
Contributor Author

@tamasvajk
Copy link
Contributor Author

C# differences job: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/457/

@hvitved The differences job shows this change didn't work out as expected in cs/self-assignment and maybe in cs/useless-cast-to-self . I'll adjust those checks.

@tamasvajk
Copy link
Contributor Author

Pushed a fix to this PR and restarted the differences job: https://jenkins.internal.semmle.com/job/Changes/job/CSharp-Differences/460/

@tamasvajk
Copy link
Contributor Author

The differences job looks good now. I don't understand why the two findings with cs/local-shadows-member and cs/missed-ternary-operator show up. They are found in the same method though.

@tamasvajk tamasvajk force-pushed the feature/enum-value-init branch from 7724f43 to a635503 Compare September 28, 2020 09:04
@hvitved hvitved merged commit 93edaa7 into github:main Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants