Page MenuHomePhabricator

[libc++] Make sure that temp_directory_path() doesn't return a path with a trailing slash
AbandonedPublic

Authored by ldionne on Mar 25 2020, 1:28 PM.

Details

Reviewers
EricWF
Group Reviewers
Restricted Project
Summary

Diff Detail

Event Timeline

ldionne created this revision.Mar 25 2020, 1:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2020, 1:28 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: EricWF.

I'm eager to see what @EricWF thinks about this.

Lets not start writing tests like this.

It breaks with the fundamental design goal of the test suite:

  • c++ test.pass.cpp should be enough to compile most tests.
  • ./a.out should be enough to run it.

Please add these tests to std/input.output/filesystems//fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp.

I don't necessarily agree.

First, no implementation currently has the behavior you describe.
Both libstdc++ and Boost filesystem return the environment variable as specified.

Also, on OS X, the TEMPDIR variable includes the trailing slash. If that's what the operating system does, why should we differ?

I also think there are some subtle differences in pathname resolution. Specifically around symlinks and when the basename doesn't exist.

For example, let's assume TEMPDIR is /this-directory-does-not-exist/. touch foo; cp foo $TEMPDIR fails because there is no such directory. Without the trailing slash it succeeds and copies foo to a new file called this-directory-does-not-exist.

In general trailing slashes are used to denote that the entity is a directory. I don't see a strong reason to differ.

But if you disagree, I'm willing to proceed with your fix, on the condition that you file a LWG issue about this proposing this behavior as standard.

Lets not start writing tests like this.

It breaks with the fundamental design goal of the test suite:

  • c++ test.pass.cpp should be enough to compile most tests.
  • ./a.out should be enough to run it.

Please add these tests to std/input.output/filesystems//fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp.

First, this test would be a non-standard extension, so it needs to stay in libcxx/test/libcxx, not libcxx/test/std. And regarding using a .sh.cpp instead of a .pass.cpp, the only reason is that I wanted to avoid using ::setenv, which is not standard. But if we're fine with using ::setenv, then I agree a .pass.cpp is a lot simpler.

I don't necessarily agree.

[...]

But if you disagree, I'm willing to proceed with your fix, on the condition that you file a LWG issue about this proposing this behavior as standard.

I replied on the bug report. I think I agree with you but I'd like to clarify a few things. Let's have the discussion on Bugzilla and I'll update this thread with the result of the discussion.

ldionne abandoned this revision.May 20 2020, 2:16 PM

Abandoning this -- see bug report for details.