-
-
Notifications
You must be signed in to change notification settings - Fork 669
Documentation for the Fast DFA Engine #22286
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
base: master
Are you sure you want to change the base?
Conversation
|
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 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 referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#22286" |
rikkimax
left a comment
There was a problem hiding this 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.
|
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. |
|
For future reference, rather than using multiple commits, we prefer that there be only one commit per PR. You can use appending ( We can squash it as part of merge, but it's better commit message wise if we don't need to. |
|
Here is the DAutoTest failure:
It is expecting all parameters to match 1:1 to the ddoc. |
5b076af to
719d376
Compare
|
I've added the missing parameters (haveFalseBody, unknownBranchTaken, etc.) to the documentation block. Pushed the fix. |
|
719d376 to
741debe
Compare
|
Please review |
|
@rikkimax can you verify, if everything is alright in this PR, so that it can be merged?. |
741debe to
819dca0
Compare
|
i have updated the changes you mentioned @rikkimax |
| } | ||
|
|
||
| /*********************************************************** | ||
| * The history of values and facts known about a variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still says history.
I have analyzed and documented the entire dmd.dfa package to make the high-performance DFA engine easier to understand and maintain.
Changes:
This should address the "bad at commenting" issue mentioned in the forum/chats!
@rikkimax