[PATCH] D29198: clang-cl: Warn about /U flags that look like filenames (PR31662)

amccarth added inline comments. Comment at: include/clang/Basic/DiagnosticDriverKinds.td:252 + InGroup>; +def note_use_dashdash : Note<"Use '--' to treat subsequent arguments as filenames">; + For Windows, wouldn't it make more

[PATCH] D30385: clang-format: Don't leave behind temp files in -i mode on Windows, PR26125, reloaded

amccarth added inline comments. Comment at: lib/Rewrite/Rewriter.cpp:455 I->second.write(File.getStream()); + prewrite(I->first); } I'm not familiar with the structure of this code, but it seems odd to me that the callback is called `prewrite`

[PATCH] D20136: Get default -fms-compatibility-version from cl.exe's version

amccarth added a comment. > and folks have to manually add mincore.lib to their link. I could load the library dynamically on demand and use GetProcAddress, but I suspect that would further degrade the performance. I could probably write my own code to find the version in the binary, but I

[PATCH] D28365: [Driver] Updated for Visual Studio 2017

amccarth added a comment. (I could be missing something because the diff doesn't have much context.) If I'm reading this right, it looks like it will no longer search the PATH in order to find cl.exe. If that's the case, then I'm mildly worried about this change in behavior. I know I have

[PATCH] D47578: Do not enforce absolute path argv0 in windows

amccarth added a comment. In https://reviews.llvm.org/D47578#1117874, @rnk wrote: > The LLDB test suite isn't in very good shape on Windows. It is complicated to > set up and build, I don't want to block this fix on @takuto.ikuta setting up > that build environment. This is a Windows-only

[PATCH] D47578: Do not enforce absolute path argv0 in windows

amccarth added a comment. I was under the impression that some tools rely on the fact that arg[0] is always expanded to an absolute path. Does this work with lldb and its test suite? https://reviews.llvm.org/D47578 ___ cfe-commits mailing list

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

amccarth added a comment. > Right now, we have to add an .exe suffix when using this switch, like > -fuse-ld=lld.exe. That's weird, because lots of lldb tests compile and link test binaries on Windows with `-fuse-ld=lld` (without the `.exe`). What makes you say the `.exe` is necessary?

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

amccarth added a comment. This must be new-ish code, since I've routinely used `-fuse-ld=lld` (without `.exe`) on Windows. Repository: rC Clang https://reviews.llvm.org/D43621 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43621: [Driver] Allow using a canonical form of '-fuse-ld=' when cross-compiling on Windows.

amccarth added a comment. In https://reviews.llvm.org/D43621#1017027, @ruiu wrote: > > That's weird, because lots of lldb tests compile and link test binaries on > > Windows with `-fuse-ld=lld` (without the `.exe`). What makes you say the > > `.exe` is necessary? > > Maybe he is using clang

[PATCH] D60094: [MSVC] If unable to find link.exe relative to MSVC, look for link.exe in the path

amccarth added inline comments. Comment at: lib/Driver/ToolChains/MSVC.cpp:493 C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found); + linkPath = TC.GetProgramPath("link.exe"); +} The comment above explains one reason why we shouldn't use

[PATCH] D67454: Start porting ivfsoverlay tests to Windows

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67454/new/ https://reviews.llvm.org/D67454

[PATCH] D68953: Enable most VFS tests on Windows

amccarth abandoned this revision. amccarth added a comment. I create a new review thread for my improved patch shortly. These changes were too wide-ranging. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68953/new/ https://reviews.llvm.org/D68953

[PATCH] D69958: Improve VFS compatibility on Windows

amccarth created this revision. amccarth added reviewers: rnk, vsapsai, arphaman, Bigcheese. Herald added subscribers: dexonsmith, hiraditya. Herald added a project: LLVM. Keys in a virtual file system can be in Posix or Windows form or even a combination of the two. Many VFS tests (and a few

[PATCH] D69958: Improve VFS compatibility on Windows

amccarth updated this revision to Diff 229144. amccarth marked an inline comment as done. amccarth added a comment. Modified comment per feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69958/new/ https://reviews.llvm.org/D69958 Files: clang/test/Index/index-module-with-vfs.m

[PATCH] D69958: Improve VFS compatibility on Windows

amccarth marked 3 inline comments as done. amccarth added a comment. Friendly ping for any VFS experts to comment. Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:657 + // slashes to match backslashes (and vice versa). + bool pathComponentMatches(llvm::StringRef

[PATCH] D69958: Improve VFS compatibility on Windows

This revision was automatically updated to reflect the committed changes. Closed by commit rG1275ab1620b6: Improve VFS compatibility on Windows (authored by amccarth). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69958: Improve VFS compatibility on Windows

amccarth marked an inline comment as done. amccarth added a comment. Thanks for the review. In D69958#1744861 , @vsapsai wrote: > There is one issue with a comment, apart of that everything looks good to me. > Looked into replacing slash and backslash

[PATCH] D69958: Improve VFS compatibility on Windows

amccarth updated this revision to Diff 229197. amccarth marked an inline comment as done. amccarth added a comment. Corrected comment about default for case sensitivity. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69958/new/ https://reviews.llvm.org/D69958 Files:

[PATCH] D69969: [SEH] Defer checking filter expression types until instantiaton

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69969/new/ https://reviews.llvm.org/D69969

[PATCH] D70701: Fix more VFS tests on Windows

amccarth marked 2 inline comments as done. amccarth added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1431 -if (IsRootEntry && !sys::path::is_absolute(Name)) { - assert(NameValueNode && "Name presence should be checked earlier");

[PATCH] D70701: Fix more VFS tests on Windows

amccarth marked 2 inline comments as done. amccarth added a comment. A (hopefully) more cogent response than my last round. I'm still hoping to hear from VFS owners. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1077-1078 +std::error_code

[PATCH] D71092: [VFS] Use path canonicalization on all paths

amccarth created this revision. amccarth added reviewers: rnk, vsapsai, arphaman. Herald added subscribers: llvm-commits, dexonsmith, hiraditya. Herald added a project: LLVM. The virtual file system had an option to eliminate dots from paths (e.g., `/foo/./bar` -> `/foo/bar`). Because of

[PATCH] D71092: [VFS] Use path canonicalization on all paths

amccarth planned changes to this revision. amccarth added a comment. Hold off on this one. It needs an explicit test for canonicalization. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71092/new/ https://reviews.llvm.org/D71092 ___

[PATCH] D68953: Enable most VFS tests on Windows

amccarth marked 2 inline comments as done. amccarth added a comment. Somehow Phabricator failed to notify me that you'd already left comments. I even searched my inbox to see if I'd just missed the notification. Nope. Nothing. I came here today to ping you for the review and discovered

[PATCH] D70701: Fix more VFS tests on Windows

amccarth marked 3 inline comments as done. amccarth added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1077-1078 +std::error_code RedirectingFileSystem::makeAbsolute(SmallVectorImpl ) const { + if (llvm::sys::path::is_absolute(Path,

[PATCH] D70701: Fix more VFS tests on Windows

amccarth created this revision. amccarth added reviewers: rnk, vsapsai, arphaman, Bigcheese. Herald added subscribers: dexonsmith, hiraditya. Herald added a project: LLVM. Since VFS paths can be in either Posix or Windows style, we have to use a more flexible definition of "absolute" path. The

[PATCH] D68736: [MSVC] Automatically add atlmfc include and lib directories as system paths.

amccarth added a comment. > This matches the behavior of cl. Are you sure? In a bare environment, cl.exe doesn't include any system paths, not even to the standard library. It actually uses the INCLUDE environment variable for those paths. Granted, VCVARSALL sets that (and other environment

[PATCH] D68953: Enable most VFS tests on Windows

amccarth created this revision. amccarth added a reviewer: rnk. Herald added subscribers: llvm-commits, hiraditya. Herald added a project: LLVM. Inconsistencies in when/if paths are cannoicalized prevented VFS tests from working on Windows. This fixes the primary cause and enables most of the

[PATCH] D70701: Fix more VFS tests on Windows

This revision was automatically updated to reflect the committed changes. amccarth marked an inline comment as done. Closed by commit rG738b5c9639b4: Fix more VFS tests on Windows (authored by amccarth). Herald added a project: clang. Changed prior to commit:

[PATCH] D73457: [Clang] Warn about 'z' printf modifier in old MSVC.

amccarth added a comment. In D73457#1842493 , @simon_tatham wrote: > Removed the special case for `MSCompatibilityVersion == 0`. If the default > compatibility setting needs to be changed, that's a separate piece of work > and should be done by someone

[PATCH] D71092: [VFS] More consistent support for Windows

amccarth added a comment. Ping? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71092/new/ https://reviews.llvm.org/D71092 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71092: [VFS] More consistent support for Windows

amccarth updated this revision to Diff 242190. amccarth marked an inline comment as done. amccarth added a comment. Addressed rnk's comments: fixed path style detection bug, and used `StringRef::endswith`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71092/new/

[PATCH] D71092: [VFS] More consistent support for Windows

amccarth marked 3 inline comments as done. amccarth added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:-1113 + sys::path::Style style = sys::path::Style::posix; + if (sys::path::is_absolute(WorkingDir.get(), sys::path::Style::windows)) { +style =

[PATCH] D71092: [VFS] More consistent support for Windows

amccarth added a comment. cc: +thakis, who expressed interest in seeing a fix for the text exempted on Windows. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71092/new/ https://reviews.llvm.org/D71092 ___ cfe-commits mailing list

[PATCH] D71092: [VFS] More consistent support for Windows

amccarth updated this revision to Diff 240240. amccarth retitled this revision from "[VFS] Use path canonicalization on all paths" to "[VFS] More consistent support for Windows". amccarth edited the summary of this revision. Herald added a subscriber: ormris. CHANGES SINCE LAST ACTION

[PATCH] D71092: [VFS] More consistent support for Windows

amccarth closed this revision. amccarth added a comment. This landed in da45bd232165eab5d6ec4f1f4f18db8644289142 It wasn't automatically closed because I misspelled "Differential Revision." CHANGES SINCE LAST ACTION

[PATCH] D71991: Fix external-names.c test when separator is \\

amccarth added a comment. It would surprise me if the different test config is cause here. The path is assembled entirely in the code, so it should be consistent. If you had a systemic problem introduced by test config, I'd expect more of the VFS tests to fail. Anyway, if you're back in

[PATCH] D71991: Fix external-names.c test when separator is \\

amccarth added a comment. I'm still not seeing how the backslashes got in there for you. They don't happen for me. It looks like these paths come (almost) directly from the temp .yaml files created by the `sed` commands at the beginning of the test, so--for these particular tests--I'd expect

[PATCH] D71991: Fix external-names.c test when separator is \\

amccarth accepted this revision. amccarth added a comment. I've made a similar change in several other tests, but this test works for me without this change. I'm trying to understand why, but I don't see any harm in landing this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms.

amccarth added a comment. In D78508#2005311 , @aaron.ballman wrote: > Personally, I don't consider those to be useless casts. They appropriately > ensure that the types match between the arguments and the format specifiers, > rather than relying on

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

amccarth created this revision. amccarth added reviewers: rnk, hans. Herald added a subscriber: aprantl. An earlier change eliminated spaces between the close brackets of nested template lists. Unfortunately that prevents the Windows debuggers from matching some types to their corresponding

[PATCH] D79223: Fix pr31836 on Windows too, and correctly handle repeated separators.

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. OK as is, but please consider re-using `llvm::sys::path` to distinguish separators. Please also check that there are no regressions in the clang VFS tests. Comment at:

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

amccarth updated this revision to Diff 263790. amccarth added a comment. Addressed feedback, specifically: - Distinction is now on CodeView generation rather than -fms-compatibility. - Tests two --std= levels plus the default one. CHANGES SINCE LAST ACTION

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

amccarth marked 2 inline comments as done. amccarth added a comment. Made the requested changes after an in-person conversation to clear up my earlier confusion. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79274/new/ https://reviews.llvm.org/D79274

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

amccarth updated this revision to Diff 262499. amccarth added a comment. Updated an existing test. Thanks for the pointer; I had gotten myself completely confused about the organization of the test directories. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79274/new/

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

amccarth added a comment. Updated test to validate the change. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79274/new/ https://reviews.llvm.org/D79274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

This revision was automatically updated to reflect the committed changes. Closed by commit rGa549c0d00486: Fix template class debug info for Visual Studio visualizers (authored by amccarth). Herald added a project: clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78508: [Clang] Allow long as size_t printf argument on 32-bit Windows platforms.

amccarth added a comment. I'll re-iterate my opinion that the casts currently required to bypass the warnings are useful (e.g., if you ever want to port the code to 64-bit). There are lots of places where the Windows API essentially required uncomfortable conversions, and we can't paper over

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

amccarth marked 2 inline comments as done. amccarth added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp:10 +// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \ +// RUN: -o - -triple=x86_64-pc-win32

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Thanks for extending this functionality to Windows! Comment at: clang/lib/DirectoryWatcher/windows/DirectoryWatcher-windows.cpp:15 +#include

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth requested changes to this revision. amccarth added a comment. This revision now requires changes to proceed. Some of this is nitpicky/opinionated, but the race condition is real. We need a reliable way to signal the watcher thread when it's time to exit. Options I see are: 1. Use

[PATCH] D89702: [clang] [msvc] Automatically link against oldnames just as linking against libcmt

amccarth accepted this revision. amccarth added a comment. It seems weird that we're implicitly adding `defaultlib` options without checking if the user specified `nodefaultlib`. But given that's already the case, I don't see a big problem with also adding oldnames.lib. Repository: rG LLVM

[PATCH] D70378: [LLD][COFF] Cover usage of LLD as a library

amccarth accepted this revision. amccarth added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70378/new/ https://reviews.llvm.org/D70378 ___ cfe-commits mailing list

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth added a comment. I'm sorry. I haven't had to time to review the entire change yet, but I thought I'd share some early feedback now and take more of a look on Monday. The high level issues on my mind: I'm wondering whether this has been overcomplicated with the overlapped IO. If the

[PATCH] D80554: [DebugInfo] Use SplitTemplateClosers (foo >) in DWARF too

amccarth marked an inline comment as done. amccarth added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:242 +// SplitTemplateClosers yields better interop with GCC and GDB (PR46052). +PP.SplitTemplateClosers = true; } sammccall wrote:

[PATCH] D80880: [clang] [MinGW] Link kernel32 once after the last instance of msvcrt

amccarth accepted this revision. amccarth added a comment. Yowza. Mingw still links against MSVCRT?! Anyway, I think the patch is fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80880/new/ https://reviews.llvm.org/D80880

[PATCH] D80554: [DebugInfo] Use SplitTemplateClosers (foo >) in DWARF too

amccarth added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:242 +// SplitTemplateClosers yields better interop with GCC and GDB (PR46052). +PP.SplitTemplateClosers = true; } So, in other words, we'll always set

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

amccarth added a comment. Does the full path to the tool end up only in the object file, or does this make it all the way to the final executable or library? Does embedding full paths affect distributed builds or build reproducibility? Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D83772: [Windows] Fix limit on command line size

amccarth added a comment. In D83772#2164685 , @max-kudr wrote: > This commit is now failing LLDB Windows buildbot > builds > http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/17702.

[PATCH] D83772: [Windows] Fix limit on command line size

amccarth added a comment. I'm jumping in since rnk is on leave and (I believe) zturner is less focused on these issues than he used to be. Thanks for tracking down the cause of this bug. I have some concerns: - We're trying to cut it right up to the hard limit. That seems an unnecessary

[PATCH] D83772: [Windows] Fix limit on command line size

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks! I know it was more work, but I think unifying the command line generation to work the same in both cases will avoid future surprises. Repository: rG LLVM Github Monorepo

[PATCH] D81794: [clang] Don't emit warn_cxx_ms_struct when MSBitfields is enabled globally

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks. This seems well-contained and the test changes look good. (rnk is on leave and unlikely to respond.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81794/new/

[PATCH] D81041: Use existing path sep style in clang::FileManager::FixupRelativePath

amccarth added a comment. Please make sure all of the clang VFS tests still pass before submitted this. VFS paths are inherently problematic with `llvm::sys::path` because they can legitimately be in a hybrid of Windows and Posix styles. Guessing the style from the first separator you see

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

amccarth added a comment. In D80833#2069874 , @thakis wrote: > The change description says something about PWD I believe that was a typo for CWD. > Please don't add code that stores absolute paths, but at the moment this > doesn't do that as far as I

[PATCH] D81041: Use existing path sep style in clang::FileManager::FixupRelativePath

amccarth added a comment. In D81041#2072396 , @ctetreau wrote: > After some further investigation, I have come to believe that the root cause > of the issue I am seeing is on line 783 of clang/lib/Lex/HeaderSearch.cpp. A > path is constructed using

[PATCH] D80833: [CodeView] Add full repro to LF_BUILDINFO record

amccarth accepted this revision. amccarth added a comment. My only real concern was about the possible reproducibility impact of introducing absolute paths into the binaries. If @thakis and @hans are satisfied this is OK, then LGTM. Comment at:

[PATCH] D83991: With MSVC, file needs to be compiled with /BIGOBJ

This revision was automatically updated to reflect the committed changes. Closed by commit rG14dde438d69c: With MSVC, file needs to be compiled with /BIGOBJ (authored by amccarth). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo

[PATCH] D103806: [SystemZ][z/OS] Pass OpenFlags when creating tmp files

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103806/new/ https://reviews.llvm.org/D103806

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth added a comment. This patch was reverted a while back because a couple DirectoryWatcher tests routinely timed out on build bots even though they work when run in isolation. I believe the problem is that, on a machine busy doing a build, the startup of the watcher thread is delayed

[PATCH] D88666: DirectoryWatcher: add an implementation for Windows

amccarth added a comment. In D88666#2825557 , @stella.stamenova wrote: > One thing we've run into in the past is that running some of these tests **on > a drive other than the OS drive** can cause weird failure. I am not sure if > that may be the case

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth added a comment. Nice catch Reid. Comment at: clang/lib/Frontend/CompilerInstance.cpp:857-858 + OS.reset(new llvm::raw_fd_ostream(Temp->FD, /*shouldClose=*/false, +Binary ? llvm::sys::fs::OF_None +

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth added a comment. I'll have to dig into my archived email tomorrow. I seem to recall some thrashing on this topic a few months ago. If I'm remembering correctly, setting the disposition to delete temporary files on Windows was causing problems with Rust builds because you can't always

[PATCH] D101479: [Driver] Support libc++ in MSVC

amccarth added a comment. In D101479#2748475 , @mstorsjo wrote: > In D101479#2748189 , @amccarth > wrote: > >> In D101479#2733354 , @mstorsjo >> wrote: >> >>> Not sure

[PATCH] D102339: [clang] On Windows, ignore case and separators when discarding duplicate dependency file paths.

amccarth added inline comments. Comment at: clang/lib/Frontend/DependencyFile.cpp:145 + StringRef SearchPath; +#ifdef _WIN32 + // Make the search insensitive to case and separators. rnk wrote: > I feel like this should somehow be a property of the input

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth added a comment. At some point, the duplicate handle must be closed. I don't see that happening. I've added an inline comment where I think it should be done. (I find it weird that duplicating the handle seems necessary.) At a high level, it seems a shame that `llvm::support::fs`

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth requested changes to this revision. amccarth added a comment. This revision now requires changes to proceed. Sorry, forgot to Request Changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://reviews.llvm.org/D102736

[PATCH] D101479: [Driver] Support libc++ in MSVC

amccarth added a comment. In D101479#2733354 , @mstorsjo wrote: > Not sure if we want the desicion between static and shared libc++ be coupled > with `/MT` and `/MD`, as one can quite plausibly want to use e.g. a static > libc++ with `/MD`. I don't

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth added a comment. Cool! This is getting much simpler. My remaining comments are mostly musings and you can tell me to buzz off. But I'd like to see @aganea's questions addressed. It does seem redundant to have to keep the file name when we already have an object that represents the

[PATCH] D102736: Fix tmp files being left on Windows builds.

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Yeah, this is good. My remaining comments are all speculations about how to improve this further in the future, but they aren't directly applicable to the goal here. Repository:

[PATCH] D100756: [llvm-rc] [4/4] Add a GNU windres-like frontend to llvm-rc

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. There's a lot going on here, but I don't see anything wrong. Thanks for the completeness of the tests and the comments, as that helps a lot in understanding what's going on here.

[PATCH] D101378: [llvm, clang] Remove stdlib includes from .h files without `std::`

amccarth added a comment. A drive-by look. Comment at: clang/include/clang/Tooling/Transformer/Parsing.h:24 #include -#include `` appears unnecessary as well. And while this doesn't require `` it does require `llvm/ADT/StringRef.h`.

[PATCH] D99200: [SystemZ][z/OS] JSON file should be text files

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99200/new/ https://reviews.llvm.org/D99200

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

amccarth added a comment. When changing an IO stream's mode from binary to text breaks only Windows, it's likely due to the fact that Windows uses CR+LF as the newline indicator. Not only are the CRs sometimes unexpected, they can also throw off code that tries to seek to a computed file

[PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

amccarth added a comment. I like the composability enabled by making `OF_Text` and `OF_CRLF` independent. I wonder, though, if there's a chokepoint where we could/should assert if the caller uses `OF_CRLF` without `OF_Text`, which would be suspicious. I'm not concerned by the number of files

[PATCH] D99580: [CLANG] [DebugInfo] Convert File name to native format

amccarth added a comment. In D99580#2660040 , @kamleshbhalui wrote: > In D99580#2659858 , @amccarth wrote: > >> It looks like the code change is for everyone, but the new test is specific >> to mingw. > > For

[PATCH] D99580: [CLANG] [DebugInfo] Convert File name to native format

amccarth added a comment. The previous discussions (that I participated in) were centered around the redirecting virtual filesystem, which creates paths in hybrid style and there is no "correct" way to make those native without taking breaking changes and making it less useful for writing

[PATCH] D97364: [docs] Add a release note for the removing of -Wreturn-std-move-in-c++11

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97364/new/ https://reviews.llvm.org/D97364

[PATCH] D88220: [C++20] P1825R0: More implicit moves

amccarth added a comment. The option is used in the wild, but not as widely as I first believed. We've already fixed a couple projects that were using it. I think the Release Note is the right solution. Thanks for doing that. I withdraw my suggestion to allow and ignore. Repository: rG

[PATCH] D97437: Rewrite MSVC toolchain discovery with VFS

amccarth added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97437/new/ https://reviews.llvm.org/D97437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

amccarth added a comment. From a Windows point of view, this LGTM. Comment at: clang/lib/Frontend/FrontendActions.cpp:841 } } The preceding block that detects the type of line separator seems ripe for factoring out into a separate function. It's a

[PATCH] D99580: [CLANG] [DebugInfo] Convert File name to native format

amccarth added a comment. Sorry, I think I've lost track of some context while I was on vacation. I don't understand why several of the tests are now unsupported on Windows. Some of those seem like important tests. If canonicalizing the file names to the platform's native style creates new

[PATCH] D100488: [SystemZ][z/OS] Add IsText Argument to GetFile and GetFileOrSTDIN

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Personally, I'm not a fan of boolean function parameters because of the inline comments necessary to make the call site understandable. But it appears to be consistent with LLVM Coding

[PATCH] D68321: Fix clang Visual Studio build instructions

This revision was automatically updated to reflect the committed changes. Closed by commit rG6e77a67171e6: Fix clang Visual Studio build instructions (authored by Loghorn, committed by amccarth). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D68321: Fix clang Visual Studio build instructions

amccarth added a comment. I'll merge it for you. Reviewers generally assume you have commit access and will submit it yourself unless tell them otherwise. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68321/new/ https://reviews.llvm.org/D68321

[PATCH] D88220: [C++20] P1825R0: More implicit moves

amccarth added a comment. The removal of `-Wreturn-std-move-in-c++11` breaks a few projects. These projects are specifying the option, which then triggers an unknown-option warning and thus `-Wx` halts the build. Would anyone object to allowing the option but silently ignoring it, at least

[PATCH] D96971: Allow but ignore `-Wreturn-std-move-in-c++11`

amccarth abandoned this revision. amccarth added a comment. Fair enough. I'm continuing the whack-a-mole game for the Chromium third-parties. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96971/new/ https://reviews.llvm.org/D96971 ___

[PATCH] D96971: Allow but ignore `-Wreturn-std-move-in-c++11`

amccarth created this revision. amccarth added reviewers: thakis, nullptr.cpp. amccarth requested review of this revision. This warning is no longer needed, but at least a few open source projects still build with this option (and `-Werror`). Accepting but ignoring the option keeps these