Skip to content
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

LVA: Add CfgSerializer.RoslynLvaWalker #9529

Merged
merged 4 commits into from
Jul 18, 2024

Conversation

sebastien-marichal
Copy link
Contributor

@sebastien-marichal sebastien-marichal commented Jul 16, 2024

The first commit is only CaYC.
I went a bit wild on cleaning...

@sebastien-marichal sebastien-marichal added Type: Tooling Tools make us productive. Sprint: LVA labels Jul 17, 2024
@mary-georgiou-sonarsource mary-georgiou-sonarsource changed the title Add CfgSerializer.RoslynLvaWalker LVA: Add CfgSerializer.RoslynLvaWalker Jul 17, 2024
new RoslynCfgWalker(writer, new RoslynCfgIdProvider()).Visit(cfg, title);
return writer.ToString();
}
public static string Serialize(RoslynLiveVariableAnalysis lva, string title = "RoslynCfg")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it makes sense but I'd change the title to "RoslynCfgLVA" to avoid confusion in the future.
Like why I get this result here and another one in another case.
Like this, it might make it clear that this is a different CFG.


protected override void WriteEdges(BasicBlock block)
{
foreach (var predecessor in lva.BlockPredecessors[block.Ordinal])
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good but we are ignoring the ControlFlowBranches and their semantics - which probably should appear as edge labels :/

Please add couple of tests with while loops, if-else and foreach so we see how it looks there.

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 made some change to include semantic information 🎉

@@ -36,9 +36,21 @@ public RoslynLvaWalker(RoslynLiveVariableAnalysis lva, DotWriter writer, RoslynC

protected override void WriteEdges(BasicBlock block)
{
foreach (var predecessor in lva.BlockPredecessors[block.Ordinal])
var lvaPredecessors = lva.BlockPredecessors[block.Ordinal];
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of duplication now :/
Maybe it's possible to change what you need here and then call the base.WriteEdges?
Maybe you can also give the predecessors are input?

Copy link
Contributor

@mary-georgiou-sonarsource mary-georgiou-sonarsource left a comment

Choose a reason for hiding this comment

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

Thre's quite some duplication now in the cfg walker.

Copy link

sonarcloud bot commented Jul 18, 2024

Copy link

sonarcloud bot commented Jul 18, 2024

Copy link
Contributor

@mary-georgiou-sonarsource mary-georgiou-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

@mary-georgiou-sonarsource mary-georgiou-sonarsource merged commit 2c93b52 into master Jul 18, 2024
20 checks passed
@mary-georgiou-sonarsource mary-georgiou-sonarsource deleted the sma/lva-cfg-serializer branch July 18, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Tooling Tools make us productive.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants