-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix for inheritance problem exposed in examples #1891
Conversation
…ind out statically that an overload for Method was missing
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'm confused about the use of the word "inherits" here. You changed all the contains
methods to inherits
with comment changes like this: from
* Return true if the specified timer should be included in the code generated for the federate.
* This means that the timer is used as a trigger in a top-level reaction that belongs to this
* federate. This also returns true if the program is not federated.
to
* Return {@code true} if this federate inherits the given timer.
But these don't mean the same thing. Inheriting a timer means that the timer is defined in a class that this reactor extends. But that does not seem to be what is being checked. Perhaps a different word is needed here?
core/src/main/java/org/lflang/federated/generator/FederateInstance.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/lflang/federated/generator/FederateInstance.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/lflang/federated/generator/FederateInstance.java
Outdated
Show resolved
Hide resolved
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.
LGTM, modulo some suggestions on comments and reintroducing initialization of local variables.
The PR was already on auto-merge, and your approval pushed it onto the merge queue. Next time, when making suggestions, please hit the "request changes" button, not the "approve" one :-) ...I can create a separate PR to make your suggested changes to the comments. |
Alternatively, we could enable a feature that requires all conversations to be resolved prior to merging. I think it might be a good idea because that gives reviewers the ability to approve and still give suggestions for (optional) changes. |
Updated comments in response to review of #1891
FederateInstance