Page MenuHomePhabricator

Improve reliability of file renaming in Windows
ClosedPublic

Authored by dyung on Mar 4 2016, 5:50 PM.

Details

Summary

Improve the reliability of file renaming in Windows by having the compiler retry the rename operation on 3 error conditions of ReplaceFileW() that it was previously bailing out on.

Diff Detail

Repository
rL LLVM

Event Timeline

dyung updated this revision to Diff 49863.Mar 4 2016, 5:50 PM
dyung retitled this revision from to Improve reliability of file renaming in Windows.
dyung updated this object.
dyung added a reviewer: llvm-commits.
dyung edited reviewers, added: Bigcheese, gbedwell; removed: llvm-commits.Mar 4 2016, 5:55 PM
dyung added a subscriber: llvm-commits.
asl added a subscriber: asl.Mar 7 2016, 12:41 PM
vsk added a subscriber: vsk.Mar 16 2016, 10:24 PM
vsk added inline comments.
lib/Support/Windows/Path.inc
275 ↗(On Diff #49863)

Does ReplaceError need to be declared a DWORD somewhere? I'm not familiar with this code, I could just be missing something.

dyung updated this revision to Diff 51006.Mar 17 2016, 6:39 PM

Fixed missing declaration identified by vsk.

rnk added a reviewer: rnk.Mar 18 2016, 8:53 AM
rnk added inline comments.Mar 18 2016, 9:02 AM
lib/Support/Windows/Path.inc
271 ↗(On Diff #51006)

These begin() calls should be data() calls.

281 ↗(On Diff #51006)

MSDN says this for ERROR_UNABLE_TO_MOVE_REPLACEMENT_2:
"The file to be replaced still exists with a different name."

Should we do anything about that?

dyung updated this revision to Diff 51124.Mar 20 2016, 1:00 AM

Changed .begin() calls to .data() calls as suggested in review.

dyung added inline comments.Mar 20 2016, 1:12 AM
lib/Support/Windows/Path.inc
281 ↗(On Diff #51124)

We felt that this situation was not possible in this case since we call ReplaceFileW with NULL as the value for lpBackupFileName, so we felt that no backup file would be made. Because of that, if this error occurs, we thought the result would be the same as the ERROR_UNABLE_TO_MOVE_REPLACEMENT, so we would handle it the same way.

rnk accepted this revision.Mar 21 2016, 9:21 AM
rnk edited edge metadata.

lgtm

lib/Support/Windows/Path.inc
281 ↗(On Diff #51124)

I can agree with that interpretation. :)

This revision is now accepted and ready to land.Mar 21 2016, 9:21 AM
This revision was automatically updated to reflect the committed changes.