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

Implement mkdtemp primitive on Windows #313

Merged
merged 9 commits into from
Jan 18, 2022
Merged

Implement mkdtemp primitive on Windows #313

merged 9 commits into from
Jan 18, 2022

Conversation

erikcorry
Copy link

No description provided.

@erikcorry erikcorry requested a review from floitsch January 17, 2022 12:57
@cla-bot cla-bot bot added the cla-signed The contributors have signed the CLA label Jan 17, 2022
Copy link
Member

@floitsch floitsch left a comment

Choose a reason for hiding this comment

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

LGTM.

Nice progress.

Comment on lines +332 to +336
bool in_standard_tmp_dir = false;
if (strncmp(prefix, "/tmp/", 5) == 0) {
in_standard_tmp_dir = true;
prefix += 5;
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to check for that.
The prefix should just be a prefix for the tmp directory.
For example, some of the directories created with lsp_repro- prefix:

lsp_repro-0uHt7f
lsp_repro-1fl2A8
lsp_repro-1I2HYr
lsp_repro-1sPJA6
lsp_repro-1vUdC9
lsp_repro-4jTjLL
lsp_repro-4v3TB7
lsp_repro-5GbI7q
lsp_repro-66nfUl
lsp_repro-6gdWvL

It's actually called "template" for the C function.
If we allow a full path in the C version, then we should probably fix that instead.

Copy link
Author

Choose a reason for hiding this comment

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

The ar test uses a full path as the prefix.

Copy link
Member

Choose a reason for hiding this comment

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

oops.
Should change the test...

Copy link
Author

Choose a reason for hiding this comment

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

After some offline discussion we agreed it was enough to document the current behaviour.

@erikcorry erikcorry merged commit adcca9d into master Jan 18, 2022
@erikcorry erikcorry deleted the windows-mkdtemp branch January 18, 2022 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The contributors have signed the CLA
Development

Successfully merging this pull request may close these issues.

2 participants