Skip to content

Conversation

@Adityazzzzz
Copy link
Contributor

I have analyzed and documented the entire dmd.dfa package to make the high-performance DFA engine easier to understand and maintain.
Changes:

  • Architecture: Added high-level comments to entry.d and structure.d explaining the O(1) single-pass design and memory model.
  • Control Flow: Documented statement.d explaining how branching and "LoopyLabels" are handled without fixed-point iteration.
  • Data Flow: Documented expression.d and analysis.d covering Transfer Functions, Confluence (Meet/Join), and Gate logic.
  • Reporting: Documented error reporting in report.d.

This should address the "bad at commenting" issue mentioned in the forum/chats!
@rikkimax

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @Adityazzzzz! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#22286"

Copy link
Contributor

@rikkimax rikkimax left a comment

Choose a reason for hiding this comment

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

Over all some good changes here thanks!

I've add suggestions, only one of which could do with a bit of a rethink.

@Adityazzzzz
Copy link
Contributor Author

I've applied all the suggested text changes and ran the pre-commit tool to fix the whitespace issues for CI. I also updated the analysis.d comment to clarify that DFAConsequence holds the specific logic.

@rikkimax
Copy link
Contributor

For future reference, rather than using multiple commits, we prefer that there be only one commit per PR.

You can use appending (--ammend) in git, then force push to change the last commit.

We can squash it as part of merge, but it's better commit message wise if we don't need to.

@rikkimax
Copy link
Contributor

Here is the DAutoTest failure:

�[1msrc/dmd/dfa/fast/analysis.d(131): �[1;33mWarning: �[mDdoc: parameter count mismatch, expected 6, got 3
void convergeStatementIf(DFALatticeRef condition, DFAScopeRef scrTrue,

It is expecting all parameters to match 1:1 to the ddoc.

@Adityazzzzz
Copy link
Contributor Author

I've added the missing parameters (haveFalseBody, unknownBranchTaken, etc.) to the documentation block. Pushed the fix.
@rikkimax

@rikkimax
Copy link
Contributor

�[1msrc/dmd/dfa/fast/expression.d(94): �[1;33mWarning: �[mDdoc: parameter count mismatch, expected 5, got 3
DFALatticeRef seeAssign(DFAVar* assignTo, bool construct, DFALatticeRef lr,

@Adityazzzzz
Copy link
Contributor Author

Please review

@Adityazzzzz
Copy link
Contributor Author

@rikkimax can you verify, if everything is alright in this PR, so that it can be merged?.

@Adityazzzzz
Copy link
Contributor Author

i have updated the changes you mentioned @rikkimax

}

/***********************************************************
* The history of values and facts known about a variable.
Copy link
Contributor

Choose a reason for hiding this comment

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

This still says history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants