Skip to content
This repository has been archived by the owner on Jan 5, 2021. It is now read-only.

Issue 1336 #1365

Closed
wants to merge 5 commits into from
Closed

Issue 1336 #1365

wants to merge 5 commits into from

Conversation

BeckmaR
Copy link
Contributor

@BeckmaR BeckmaR commented May 10, 2017

Resolves #1336 by calculating and applying defines for all states that can be used in the code instead of "magic" indices of the StateConfVector.

@BeckmaR BeckmaR mentioned this pull request May 10, 2017
@@ -0,0 +1,46 @@
package org.yakindu.sct.generator.c
Copy link
Member

Choose a reason for hiding this comment

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

Missing Copyright header

import org.yakindu.sct.model.sexec.ExecutionState
import org.yakindu.sct.model.sexec.naming.INamingService

class StateConfVectorIndexCalculator {
Copy link
Member

Choose a reason for hiding this comment

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

Missing author tag

protected Map<ExecutionState, Integer> indexMap
protected Map<ExecutionState, String> defineMap

def Integer getIndex(ExecutionState it) {
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the Map. Its just a call to stateVector.offset, there is no need to cache the result for this. Then the IndexCalculator can be used stateless and as a singleton.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree!

  • The class does not calculate anything as the calculation of the index is already calculated.
  • it is effectively just a shortcut access extension that allows to write it.index instead of it.stateVector.offset.
  • it.stateVector.offset is more precise about what is really meant.

So - please remove this class

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 whole class? @andreasmuelder only proposed to remove the map, which I agree with. The class does make sense because the name of the define is used in two locations, FlowCode and StatemachineHeader. Not having this class or at least the function in some other class would introduce copied code.

Copy link
Contributor

@terfloth terfloth left a comment

Choose a reason for hiding this comment

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

from comments:

  • remove StateConfVectorIndexCalculator
  • add copyright header

protected Map<ExecutionState, Integer> indexMap
protected Map<ExecutionState, String> defineMap

def Integer getIndex(ExecutionState it) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree!

  • The class does not calculate anything as the calculation of the index is already calculated.
  • it is effectively just a shortcut access extension that allows to write it.index instead of it.stateVector.offset.
  • it.stateVector.offset is more precise about what is really meant.

So - please remove this class

@BeckmaR BeckmaR dismissed stale reviews from andreasmuelder and terfloth May 23, 2017 07:57

Outdated

@BeckmaR BeckmaR requested a review from andreasmuelder May 23, 2017 07:57
@BeckmaR BeckmaR closed this May 23, 2017
@andreasmuelder andreasmuelder deleted the issue_1336 branch July 10, 2017 13:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants