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

2039 compute jump targets #2099

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

2039 compute jump targets #2099

wants to merge 8 commits into from

Conversation

fm-117
Copy link
Contributor

@fm-117 fm-117 commented Dec 16, 2021

Cancels and replaces #2052.

The cycle detection will be done outside of the parser but now we build a JumpTarget for each PERFORM or GO TO instruction. Targets for these jump instructions are stored in the resulting graph object and publicly accessible.

@fm-117 fm-117 self-assigned this Dec 16, 2021
@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Dec 16, 2021

namespace TypeCobol.Analysis.Graph
{
public partial class ControlFlowGraph<N, D>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think partial class are harder to read.
As this class and some others are moving to another source file, isn't not possible to declare them as top class rather than under public partial class ControlFlowGraph<N, D> ?

Copy link
Contributor Author

@fm-117 fm-117 Jan 7, 2022

Choose a reason for hiding this comment

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

I tried at first to turn them into regular classes but I stopped because of the generic parameters. As nested classes of ControlFlowGraph<N, D> they are implicitly generic. Sentence references BasicBlock<N, D> so all inheritors and users of Sentence must be generic too at this point.

/// </summary>
public class JumpTarget
{
private readonly List<Sentence> _sentences;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to store sentences ?
The target of a perform or a go to is a paragraph, a section or a range of paragraph/section. So the references to procedure(s) should be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sentence accumulation mechanism in case of a range is not trivial, as we need to compute the sentence list for the perform block cloning, we compute the list here and store for re-use. Also the listing of procedure themselves is done through this accumulation process so we get both sentences and procedure in one go.

@trafico-bot trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Jan 6, 2022
@fm-117 fm-117 requested a review from smedilol January 7, 2022 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ Changes requested Pull Request needs changes before it can be reviewed again
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants