This is an archive of the discontinued LLVM Phabricator instance.

Let normalize() for posix style convert backslash to slash unconditionally.
ClosedPublic

Authored by thakis on May 1 2020, 2:06 PM.

Details

Summary

Currently, normalize() for posix replaces backslashes to slashes, except
that two backslashes in sequence are kept as-is.

clang calls normalize() to convert \ to / is microsoft compat mode. This
generally works well, but a path like "c:\\foo\\bar.h" with two
backslashes doesn't work due to the exception in normalize().

These paths happen naturally on Windows hosts with e.g.
#include __FILE__, and them not working on other hosts makes it
more difficult to write tests for this case.

The special case has been around without justification since this code
was added in r203611 (since then moved around in r215241 r215243). No
integration tests fail if I remove it.

Try removing the special case.

Diff Detail

Event Timeline

thakis created this revision.May 1 2020, 2:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2020, 2:06 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

What happens if you encounter a "\t" as a string? This would convert that to a "/t" would it not? Although, I suppose that in practice the treatment of escaped characters is not important. I think I still prefer the } else { here over the early return.

thakis updated this revision to Diff 261980.May 4 2020, 6:16 PM

restore else

thakis added a comment.May 4 2020, 6:17 PM

What happens if you encounter a "\t" as a string? This would convert that to a "/t" would it not? Although, I suppose that in practice the treatment of escaped characters is not important. I think I still prefer the } else { here over the early return.

"\t" is converted to "/t". That's also the behavior before this change though – only '\' followed by '\' was handled specially before, not '\' followed by anything else.

I restored the else and removed the early return.

compnerd accepted this revision.May 5 2020, 8:30 AM

Seems reasonable, especially since none of the other tests break. I don't fully remember the reason for the special case, and if it ever turns up, we can address it then.

This revision is now accepted and ready to land.May 5 2020, 8:30 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2020, 11:20 AM