-
Notifications
You must be signed in to change notification settings - Fork 1
Wrap debug messages with a conditional compilation check. #23
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
Conversation
autogenerated.ts
Outdated
| return result; | ||
| }; | ||
|
|
||
| simulatorSendData(); |
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.
Also added this to the default autogenerated.ts, assuming it is needed, but not really tested it with the simulator itself.
Happy to remove it if it's better without it.
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.
We don't need this line. The simulatorRegister() call kicks things off. When the simulator is rendered it asks this extension for the data, so simulatorSendData() is only triggered in response.
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.
I think this line might still be generated in the autogenerated.ts file (or at least the files generated in the ml4f-test-header branch), should it be removed from there as well?
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.
Yes, good catch. Needs testing after the change, but I'm confident that's the right thing to do.
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.
Let's remove the added simulatorSendData(); line.
The other changes look good.
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.
Thanks. LGTM.
Very minor update so that the bigger blocks with
ifstatements only to print debug info are 100% not compiled whenML_DEBUG_PRINTis disabled (the compiler should take care of that, but this little extra check doesn't cost anything).No rush, can sit here until you have to for review.