2022 abstract int multi branch context #2024
Conversation
| private class CfgAfterIterativePerformProcedureTransformer : ICfgTransform<Node, D> | ||
| { | ||
| private HashSet<BasicBlockForNodeGroup> _visitedGroups; | ||
| private ControlFlowGraphBuilder<D> Builder; |
There was a problem hiding this comment.
naming _builder ?
Edit: not required, see my other remarks.
There was a problem hiding this comment.
The new field _cfg is not used (as the graph is already available through the callback arguments). Please remove it.
TypeCobol.Analysis/Cfg/ControlFlowGraphBuilder.CfgAfterIterativePerformProcedureTransformer.cs
Outdated
Show resolved
Hide resolved
| BasicBlock<Node, D> IMultiBranchContext<Node, D>.OriginBlock => this.OriginBlock; | ||
|
|
||
| BasicBlock<Node, D> IMultiBranchContext<Node, D>.RootBlock => this.RootBlock; | ||
|
|
||
| IList<BasicBlock<Node, D>> IMultiBranchContext<Node, D>.Branches => this.Branches; | ||
|
|
||
| IList<int> IMultiBranchContext<Node, D>.BranchIndices => this.BranchIndices; |
There was a problem hiding this comment.
Why the explicit interface implementation ? You can publicly implement the getter and keep the setter internal.
There was a problem hiding this comment.
MultiBranchContext is an internal class of ControlFlowGraphBuilder, so public members are accesed via a public interface IMultiBranchContext<Node, D>, explicit interface are use to pubish public attribute that are declared with an internal type or an embedded type.
There was a problem hiding this comment.
But why mix both implementation styles then ? Some of the properties are publicly implemented and those 4 are treated differently. To summarize my view on this:
- do not create the interface type, it is not relevant
- make
MultiBranchContextpublic - check and change visbility for members: make members that should be accesible from outside public and everything else should be internal
There was a problem hiding this comment.
Ok as seen together, this is a trick to avoid exposing the BasicBlockForNode class, so we can keep it like that except for the last one which can easily be converted into public implementation.
| /// <param name="branchToNext">True if the CurrentBlock must be linked to the next branch also, false otherwise</param> | ||
| /// <param name="nextBlock">The next block for all branches</param> | ||
| internal void End(bool branchToNext, BasicBlockForNode nextBlock) | ||
| internal void End(ControlFlowGraphBuilder<D> builder, bool branchToNext, BasicBlockForNode nextBlock) |
There was a problem hiding this comment.
In fact, the builder is not required in the successive calls. We need the graph but not the builder. So change the different usages, replacing builder by graph and this also will allow to remove the Builder field in CfgAfterIterativePerformProcedureTransformer.
There was a problem hiding this comment.
You kept the builder arg here. I checked the whole class again, it does not need the builder, only the graph.
There was a problem hiding this comment.
sorry done in ed54134
trafico-bot
bot
added
⚠️ Changes requested
TypeCobol.Analysis/Cfg/ControlFlowGraphBuilder.BasicBlockForNodeGroup.cs
Show resolved
Hide resolved
TypeCobol.Analysis/Cfg/ControlFlowGraphBuilder.CfgAfterIterativePerformProcedureTransformer.cs
Outdated
Show resolved
Hide resolved
trafico-bot
bot
added
🔍 Ready for Review
| cfg.SuccessorEdges.Add(iterativeGroup.Group.First.Value); | ||
| block.SuccessorEdges.Add(entranceEdge); | ||
| System.Diagnostics.Debug.Assert(block.Context == null); | ||
| //If the assert above fails (it should not be case), then it will be necessay to exeute the commented code bellow. |
There was a problem hiding this comment.
typos: If the assert above fails (it should not be the case), then it will be necessary to execute the commented code below.
| BasicBlock<Node, D> IMultiBranchContext<Node, D>.OriginBlock => this.OriginBlock; | ||
|
|
||
| BasicBlock<Node, D> IMultiBranchContext<Node, D>.RootBlock => this.RootBlock; | ||
|
|
||
| IList<BasicBlock<Node, D>> IMultiBranchContext<Node, D>.Branches => this.Branches; | ||
|
|
||
| IList<int> IMultiBranchContext<Node, D>.BranchIndices => this.BranchIndices; |
There was a problem hiding this comment.
But why mix both implementation styles then ? Some of the properties are publicly implemented and those 4 are treated differently. To summarize my view on this:
- do not create the interface type, it is not relevant
- make
MultiBranchContextpublic - check and change visbility for members: make members that should be accesible from outside public and everything else should be internal
What have been done
All Mutli Branch Context information or accessible by an interface IMultiBranchContext from a BasicBlock.