Skip to content
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

Add mkdirTemp to System module #536

Merged
merged 12 commits into from
Oct 6, 2022

Conversation

briandowns
Copy link
Contributor

Signed-off-by: Brian Downs brian.downs@gmail.com

Well detailed description of the change :

Adds the ability to create temporary directories.

Resolves: #

Type of change:

  • Bug fix

  • New feature

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Housekeeping

  • Tests have been updated to reflect the changes done within this PR (if applicable).

  • Documentation has been updated to reflect the changes done within this PR (if applicable).

Preview (Screenshots) :

@briandowns briandowns changed the title add mkdirTemp to System module [WIP] - add mkdirTemp to System module Oct 1, 2022
@briandowns briandowns changed the title [WIP] - add mkdirTemp to System module Add mkdirTemp to System module Oct 2, 2022
strcpy(tmpl, "XXXXXX");
}

char *tmpDir = mkdtemp(tmpl);
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth using the Result type here as mkdtemp can fail. In those situations it would be good to give some indication as to why it failed, we can do that by unwrapping a Result type

char *tmpl = {0};

if (template[0] != '\0') {
tmpl = malloc(strlen(template)+1);
Copy link
Member

Choose a reason for hiding this comment

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

I try to avoid uses of malloc / alloc etc where possible and try to use the memory macros, this means if in the future we ever needed to change the memory allocator it's a simple switch.

We'll need to ensure we're freeing things up too since on success where creating a new string rather than inheriting the already alloc'd one.

Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Signed-off-by: Brian Downs <brian.downs@gmail.com>
Comment on lines 180 to 186
if (template[0] != '\0') {
tmpl = ALLOCATE(vm, char, strlen(template));
strcpy(tmpl, template);
} else {
tmpl = ALLOCATE(vm, char, PATH_MAX);
strcpy(tmpl, "XXXXXX");
}
Copy link
Member

Choose a reason for hiding this comment

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

We need to allocate for the terminating char too. My thinking here also is that if no template is passed the directory would be made via the 6 digits of XXXXXX template right? Doesn't that mean we only need to allocate for these 6 + \0?

Suggested change
if (template[0] != '\0') {
tmpl = ALLOCATE(vm, char, strlen(template));
strcpy(tmpl, template);
} else {
tmpl = ALLOCATE(vm, char, PATH_MAX);
strcpy(tmpl, "XXXXXX");
}
if (template[0] != '\0') {
tmpl = ALLOCATE(vm, char, strlen(template) + 1);
strcpy(tmpl, template);
} else {
tmpl = ALLOCATE(vm, char, 7);
strcpy(tmpl, "XXXXXX");
}

Copy link
Member

Choose a reason for hiding this comment

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

We could also probably keep reference to the length rather than calculating again via strlen in the takeString call

Signed-off-by: Brian Downs <brian.downs@gmail.com>
@Jason2605
Copy link
Member

I see it's failing on the ubuntu machines, I'll need to give that a test and see whats going on but I don't have access to a machine until later on in the week now sorry!

@Jason2605
Copy link
Member

Nevermind, found the issue! The test didn't setup the template to end with XXXXXX so it was retuning an error result, at this point the memory wasn't being free'd! I've also changed a couple things (rather than an empty string allow no template to be passed for ease) and added a few notes to the docs page. Think we should be all good on this PR now, thank you!

@Jason2605 Jason2605 merged commit 1908cdd into dictu-lang:develop Oct 6, 2022
@briandowns
Copy link
Contributor Author

Thanks. Been swamped this week. Was gonna get back to it this weekend.

@briandowns briandowns deleted the add_more_functions_to_system branch October 6, 2022 16:30
@Jason2605
Copy link
Member

No worries at all! I'd just noticed the issue after another quick scan of the PR and thought I'd get it in. Appreciate the PRs you've been opening!

@Jason2605 Jason2605 mentioned this pull request Nov 7, 2022
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.

None yet

2 participants