[PATCH] D122766: [clang] Use forward slash as the path separator for Windows in __FILE__, __builtin_FILE(), and std::source_location

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

In D122766#3420925 <https://reviews.llvm.org/D122766#3420925>, @thakis wrote:

> Do you know what cl.exe does to paths in pdb files? Does it write mixed-slashiness for foo/bar.h includes, or does it write backslashes throughout?

Looks like just backslashes:

  cl /nologo /Zi a.cc && a.exe
  llvm-pdbutil.exe dump --files \src\tmp\a.pdb | grep foo
  - (MD5: B97B3B57F55C87D06A0379A4FBCA51F0) C:\src\tmp\foo\a.h

(To make sure llvm-pdbutil didn't canonicalize, I looked at the pdb in vim and the backslashes are there.)

It appears that lld does the same.

> Windows can handle slashes, but several tools can't. I worry that if we do something different than cl, some random corner case might break (dbghelp, or some source server thing or some custom debug info processor somewhere).

Since cl is already mixing back and forward slashes, any tools consuming __FILE__ can presumably handle both. So diverging msvc by canonicalizing here doesn't seem that risky, we just have to decide in which direction.



================
Comment at: clang/lib/AST/Expr.cpp:2193
     SmallString<256> Path(PLoc.getFilename());
     Ctx.getLangOpts().remapPathPrefix(Path);
+    if (Ctx.getTargetInfo().getTriple().isOSWindows()) {
----------------
ayzhao wrote:
> hans wrote:
> > I was going to say perhaps we should put a comment about why Windows needs different treatment, but as we'd need to put the comment in three different places, maybe this (remapPathPrefix() and make_preferred()) should be factored out to a utility function?
> Extracting these lines into a separate function (say, for example, `handle_file_macro(...)`?) makes sense. However, I'm not sure which file this function would belong:
> 
> * The function would need to accept a `LangOpts` and a `TargetInfo` object. Both of these classes are Clang specific.
> * It shouldn't belong in `llvm::sys::path::Style` because the file declaring that namespace exists in the llvm directory, which shouldn't depend on things in Clang.
> * We could put it in `LangOpts`, but `TargetInfo.h` includes `LangOpts.h`, so this would create a circular dependency.
> * `TargetInfo` doesn't make much sense as it doesn't have any code generation / language specific logic.
> 
> WDYT?
It would have to be somewhere in Clang.
Maybe it could be a method in Preprocessor.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122766/new/

https://reviews.llvm.org/D122766