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

ayzhao updated this revision to Diff 427771. ayzhao added a comment. Implement -ffile-reproducible Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122766/new/ https://reviews.llvm.org/D122766 Files: clang/include/clang/Basic/LangOptions.h

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

ayzhao added a subscriber: rsmith. ayzhao added a comment. In D122766#3451775 , @hans wrote: > In D122766#3450298 , @ayzhao wrote: > >> So, the general consensus seems to be that we should use backslashes when

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

hans added a comment. In D122766#3450298 , @ayzhao wrote: > So, the general consensus seems to be that we should use backslashes when > targeting Windows. > > I implemented using only backslashes for Windows; however, >

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

dexonsmith added a comment. In D122766#3450348 , @dexonsmith wrote: > - Only canonicalize `__FILE__` for the target platform when there's a > command-line flag that suggests it's okay. I suggest adding > `-ffile-reproducible`, which could be implied

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

dexonsmith added a comment. In D122766#3450298 , @ayzhao wrote: > So, the general consensus seems to be that we should use backslashes when > targeting Windows. > > I implemented using only backslashes for Windows; however, >

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

ayzhao added a comment. So, the general consensus seems to be that we should use backslashes when targeting Windows. I implemented using only backslashes for Windows; however, clang/test/SemaCXX/destructor.cpp

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

thakis added a comment. Normalizing to `\` seems better to me than normalizing to `/` too. I abstractly like changing as little as necessary at every stage, which in this case would mean changing just the slashiness of slashes that clang itself adds, so I'm still weakly in favor of that. But I

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

dexonsmith added a comment. In D122766#3429552 , @hans wrote: >> My feeling is that the default behavior on Windows needs to be to use >> backslashes and not forward slashes. > > Okay, how would folks feel about always canonicalizing `__FILE__` etc. to

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

hans added a comment. > My feeling is that the default behavior on Windows needs to be to use > backslashes and not forward slashes. Okay, how would folks feel about always canonicalizing `__FILE__` etc. to use //backslashes// when targeting Windows? Repository: rG LLVM Github Monorepo

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

aaron.ballman added a comment. In D122766#3419303 , @hans wrote: > Maybe Martin or Aaron have opinions here too? Naively, this seems wrong to me. Yes, Windows sometimes allows you to use forward slashes, but that is not the path separator which Windows

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

ayzhao updated this revision to Diff 420350. ayzhao added a comment. Herald added a subscriber: dexonsmith. Extract logic into a separate function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122766/new/ https://reviews.llvm.org/D122766 Files:

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

hans added a comment. In 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

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

thakis added a comment. In D122766#3421410 , @mstorsjo wrote: > In D122766#3420925 , @thakis wrote: > >> Windows can handle slashes, but several tools can't. I worry that if we do >> something different than cl,

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

mstorsjo added a comment. In D122766#3420925 , @thakis wrote: > 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

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

thakis added a comment. 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? Windows can handle slashes, but several tools can't. I worry that if we do something different than cl, some random

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

mstorsjo added a comment. > When targeting Windows, the path separator used when targeting Windows > depends on the build environment when Clang _itself_ is built. This leads to > inconsistencies in Chrome builds where Clang running on non-Windows > environments uses the forward slash (/) path

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

ayzhao added inline comments. Comment at: clang/lib/AST/Expr.cpp:2193 SmallString<256> Path(PLoc.getFilename()); Ctx.getLangOpts().remapPathPrefix(Path); +if (Ctx.getTargetInfo().getTriple().isOSWindows()) { hans wrote: > I was going to say perhaps

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

hans added reviewers: mstorsjo, aaron.ballman. hans added a comment. Maybe Martin or Aaron have opinions here too? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122766/new/ https://reviews.llvm.org/D122766

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

hans added a comment. > we should have a flag that controls which slash direction to use on windows > triples, since different people will want different things. > And since most people who use clang-cl run it on Windows, the default for > that flag should imho stay a backslash (but projects

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

thakis added a comment. (I don't think the argument about '\' in include lines applies to this patch (?)) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122766/new/ https://reviews.llvm.org/D122766 ___

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

thakis added a comment. As said elsewhere, I think this is too simplistic. It's good to make this dependent on the triple, instead of on the host platform. But we should have a flag that controls which slash direction to use on windows triples, since different people will want different

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

ayzhao updated this revision to Diff 419277. ayzhao added a comment. Remove errant diff Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122766/new/ https://reviews.llvm.org/D122766 Files: clang/lib/AST/Expr.cpp

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

ayzhao created this revision. Herald added subscribers: frasercrmck, luismarques, apazos, sameer.abuasal, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, niosHD, sabuasal, simoncook, johnrusso, rbar, asb, hiraditya, arichardson. Herald