Skip to content

1505 Generalise Parameter Access#1647

Merged
trisyoungs merged 6 commits into
developfrom
1505-generalise-parameter-access
Sep 8, 2023
Merged

1505 Generalise Parameter Access#1647
trisyoungs merged 6 commits into
developfrom
1505-generalise-parameter-access

Conversation

@trisyoungs
Copy link
Copy Markdown
Member

@trisyoungs trisyoungs commented Aug 24, 2023

This PR simplifies parameter (ExpressionVariable) creation, storage, and handling, moving it all to the ProcedureNode base class rather than requiring override functions in any node type that required to use them.

To handle the automatic prefixing of ExpressionVariables in the parameters_ vector with the node's name (necessitating keeping a record of the original name), a couple of approaches were initially tried and discounted:

  1. Modifying the storage of parameters_ from std::vector<std::shared_ptr<ExpressionVariable>> to std::vector<std::pair<std::shared_ptr<ExpressionVariable>,std::string>> with the string component storing the original, base name of the variable for reference - this has the effect of "polluting" a lot of other routines using the parameters, and so was abandoned.
  2. Determining the original basename of the variable by searching the string for the last . (if it existed). However this didn't feel particularly robust and required other routines to determine whether or not a prefix existed.

So, the preferred solution in the end was to extend ExpressionVariable to keep track of both it's original name and any applied prefix. This way, impact on the rest of the code is kept to a minimum.

Closes #1505.

@trisyoungs trisyoungs force-pushed the 1505-generalise-parameter-access branch 2 times, most recently from 6e50974 to 10526bb Compare August 30, 2023 20:19
@trisyoungs trisyoungs force-pushed the 1505-generalise-parameter-access branch from 10526bb to fed1682 Compare September 4, 2023 11:08
@trisyoungs trisyoungs marked this pull request as ready for review September 4, 2023 13:03
Copy link
Copy Markdown
Contributor

@rhinoella rhinoella left a comment

Choose a reason for hiding this comment

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

All looks great, it really cleans up the code nicely.

@trisyoungs trisyoungs merged commit e3f2130 into develop Sep 8, 2023
@trisyoungs trisyoungs deleted the 1505-generalise-parameter-access branch September 8, 2023 13:02
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.

Generalise Parameter Access for Nodes

2 participants