Skip to content

Conversation

tynanford
Copy link
Contributor

This PR is in place of #117 so the git merging/commit history isn't messed up. I branched off the latest in master, cherry-picked @madelinespark 's two commits, and worked off that.

Madeline's commits already converted addReccasterEnvVars to a linked list implementation and updated all the unit tests.

@tynanford tynanford requested a review from simon-ess October 6, 2025 19:31
* itemDesc is string to describe what is being added (for error messages)
* defaultArray is an optional arg for a default list to also check for duplicates
*/
static void addToReccasterLinkedList(caster_t* self, int argc, char **argv, ELLLIST* list, const char* funcName, const char* itemDesc, const char** defaultArray)
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see why this signature is used, although I don't love it.

One thought that might clean up a bunch of the code and improve this: at the moment for the default_env array we treat this as separate from the defined ones. Why not start the caster initialisation with the default array? That is, we could actually start with

addToReccasterLinkedList(caster, count default_env_vars, addReccasterEnvVars", "Default Environment variable");

or something like that. This would allow you to get rid of the last argument as well as the if(defaultArray) block.

Copy link
Contributor Author

@tynanford tynanford Oct 14, 2025

Choose a reason for hiding this comment

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

That is a good idea. I went ahead and renamed the extra_envs linked list to just envs and it now contains all the defaults and any user added env vars. Wondering if I need to grab the lock in casterInit before modifying self->envs to add the defaults?

Also moved the argc/argv stuff back to the original functions and re-named the args to addToReccasterLinkedList to make it more generic. Does that make sense? should any other code be pulled out of addToReccasterLinkedList?

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 Security Hotspots
44.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

3 participants