Page MenuHomePhabricator

Print correctly dependency paths on Windows
ClosedPublic

Authored by xbolva00 on Sep 10 2018, 12:23 AM.

Details

Summary

Before:
main.o: main.c ../include/lib\test.h

After:
main.o: main.c ../include/lib/test.h

Fixes PR38877

Diff Detail

Repository
rC Clang

Event Timeline

xbolva00 created this revision.Sep 10 2018, 12:23 AM
xbolva00 updated this revision to Diff 164620.Sep 10 2018, 12:29 AM

Remove some dead code

xbolva00 edited the summary of this revision. (Show Details)Sep 10 2018, 12:30 AM
xbolva00 retitled this revision from Print corrent dependency path on Windows to Print correctly dependency paths on Windows.Sep 10 2018, 12:38 AM

What prints this? How do you exercise this codepath?

What prints this? How do you exercise this codepath?

DFGImpl::OutputDependencyFile() -> for (StringRef File : Files)

I mean in practice. What command do i run to hit this and what will the
output look like? Can you paste some terminal output showing the command
and output?

Please see PR38877.

Alright I see it. The reason I'm asking is because after your change you're now printing main.o: main.c ../include/lib/test.h. But I wonder if you should instead be printing main.o: main.c ..\include\lib\test.h. It seems like paths that we show to the user should be in the native path style.

lib/Frontend/DependencyFile.cpp
389

This looks like a dangling reference -- undefined behavior.

Alright I see it. The reason I'm asking is because after your change you're now printing main.o: main.c ../include/lib/test.h. But I wonder if you should instead be printing main.o: main.c ..\include\lib\test.h. It seems like paths that we show to the user should be in the native path style.

Possibly, or should we follow GGC behavior here? GCC prints main.o: main.c ../include/lib/test.h. But conversion to native paths sounds fine for me as well.

clang-cl in general tries to do the thing that makes native Windows developers the most comfortable, while gcc in general tries to do the thing that makes Unix developers most comfortable. So I think we should probably use \ here. If anyone complains we can revisit.

xbolva00 updated this revision to Diff 165012.Sep 11 2018, 11:37 PM

Convert paths to platform native paths

xbolva00 updated this revision to Diff 165052.Sep 12 2018, 3:23 AM
  • Fixed failing test

Lgtm

xbolva00 updated this revision to Diff 165204.Sep 12 2018, 11:49 PM

Adjusted regexes in two more tests

This revision was not accepted when it landed; it landed in state Needs Review.Sep 13 2018, 7:28 AM
Closed by commit rC342139: Print correctly dependency paths on Windows (authored by xbolva00, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.