Page MenuHomePhabricator

Fix createUniqueFile() to do what it is documented to do: create the unique file in a temporary directory if the model isn't absolute. Fix the callers of createUniqueFile() which were affected by this change. Add test cases to verify patch.
AbandonedPublic

Authored by dirty on Nov 3 2015, 10:16 PM.

Details

Summary

Fix createUniqueFile() to do what it is documented to do: create the unique file in a temporary directory if the model isn't absolute. Fix the callers of createUniqueFile() which were affected by this change. Add test cases to verify patch.

Diff Detail

Event Timeline

dirty updated this revision to Diff 39165.Nov 3 2015, 10:16 PM
dirty retitled this revision from to Fix createUniqueFile() to do what it is documented to do: create the unique file in a temporary directory if the model isn't absolute. Fix the callers of createUniqueFile() which were affected by this change. Add test cases to verify patch..
dirty updated this object.
dirty added reviewers: chapuni, chfast, bkramer.
dirty added a subscriber: llvm-commits.
chfast edited edge metadata.Nov 4 2015, 12:17 AM

I'm not sure about the change. I think both cases (creating a file in the current and in the system temp directory) should be supported.

The example in createUniqueFile() docs should be updated too.

unittests/Support/Path.cpp
509

You need to use path::append here. A path returned by system_temp_directory() never has trailing separator.

dirty updated this revision to Diff 39297.Nov 4 2015, 5:13 PM
dirty updated this object.
dirty edited edge metadata.
  • Integrate feedback from chfast: use path::append() to guarantee path separator is present.
dirty added a comment.Nov 4 2015, 5:13 PM

Well, the current interface for createUniqueFile() doesn't allow me to select current or temp directory behavior. Are you proposing that I change it?

Because if you wanted to create a file in the current directory, you could just use fs::make_absolute() and pass it relative path. That's what I did for all the clients of createUniqueFile() who wanted a file in the current directory.

What do you mean by "the example in createUniqueFile() docs should be updated too"?

unittests/Support/Path.cpp
509

Strange. On my system (OS X) system_temp_directory() has a trailing separator. But I can see why I might not be able to rely on that on all platforms. I've updated the test case to use append.

chfast added a comment.Nov 5 2015, 2:00 AM

About the example. Currently it is:
clang-%%-%%-%%-%%-%%.s => clang-a0-b1-c2-d3-e4.s.
With your changes it should be something like:
clang-%%-%%-%%-%%-%%.s => /tmp/clang-a0-b1-c2-d3-e4.s.

In general we have 3 options here:

  1. Leave the implementation as it was, fix the docs. There are definitely use cases (exposed by your patch) that would benefit from that.
  2. Apply your patch. I suspect it is the case you are interested in.
  3. Try to support 1 and 2. It can be by adding additional param to the function or by adding similar function.

I'm not sure which solution is the best so I would like someone else to comment on that.

unittests/Support/Path.cpp
509

I was quite sure the docs of system_temp_directory() stated that it would not have the trailing /. I will look at that later, but I cannot test OSX myself.

I would say having the trailing dir separator is at least not guaranteed.

rafael requested changes to this revision.Nov 5 2015, 8:04 AM
rafael added a reviewer: rafael.
rafael added a subscriber: rafael.

The documentation should be changed.

The common use of this is to create a temp file that is then renamed. There is no point is having to form an absolute path to do that.

This revision now requires changes to proceed.Nov 5 2015, 8:04 AM
dirty abandoned this revision.Nov 5 2015, 3:47 PM

Based on feedback from Rafael, I'll update the documentation and abandon this patch.