[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

dexonsmith added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:273 - if (Status.getName() == Filename) { + if (!Status.IsVFSMapped) { // The name matches. Set the FileEntry. An incremental patch you could try would be: ``` lang=c++ if (Sta

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

dexonsmith added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:273 - if (Status.getName() == Filename) { + if (!Status.IsVFSMapped) { // The name matches. Set the FileEntry. bnbarham wrote: > bnbarham wrote: > > dexonsmith wrote: > > > An

[PATCH] D112647: [clang-apply-replacements] Correctly handle relative paths

dexonsmith added inline comments. Herald added a project: All. Comment at: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:178 } +SM.getFileManager().getFileSystemOpts().WorkingDir = PrevWorkingDir; }; This looks incorrec

[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

dexonsmith accepted this revision. dexonsmith 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/D122549/new/ https://reviews.llvm.org/D122549 __

[PATCH] D121375: [clang] NFC, move the utility function CompilerInvocation::setLangDefaults to LangOptions.h

dexonsmith requested changes to this revision. dexonsmith added a comment. This revision now requires changes to proceed. This makes sense to me! A few comments inline. Also, the description doesn't talk about timeline for removing the wrapper. Ideally we wouldn't leave behind the wrapper behind

[PATCH] D123103: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped"

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. In D123103#3428298 , @bnbarham wrote: > This broke crash reproducers in very specific circumstances, see > https://reviews.llvm.org/D123104. I

[PATCH] D123104: [Modules] Use looked-up filename when looking for module maps

dexonsmith added reviewers: benlangmuir, bruno. dexonsmith added a comment. This LGTM, with a couple of comments (on the comments!) inline. > Upstreamed from apple#llvm-project 72cf785051fb1b3ef22eee4dd33366e41a275981. Note, for some extra background: - This was out-of-tree because it seemed "h

[PATCH] D123104: [Modules] Use looked-up filename when looking for module maps

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. (Forgot to click "accept".) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123104/new/ https://reviews.llvm.org/D123104

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

dexonsmith added inline comments. Comment at: llvm/include/llvm/Support/BLAKE3.h:38 +/// A class that wraps the BLAKE3 algorithm. +template class BLAKE3 { public: The visual noise of `BLAKE3<>` everywhere is a bit unfortunate. Another option: ``` lang=c++ // H

[PATCH] D123104: [Modules] Use looked-up filename when looking for module maps

dexonsmith added inline comments. Comment at: clang/include/clang/Lex/HeaderSearch.h:758 + bool IsSystemHeaderDir, + StringRef FileName = ""); benlangmuir wrote: > This parameter could use a commen

[PATCH] D123144: FileManager: std::map => BumpPtrAllocator + DenseMap of pointers

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

[PATCH] D123144: FileManager: std::map => BumpPtrAllocator + DenseMap of pointers

dexonsmith added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:318 FileEntryRef ReturnedRef(*NamedFileEnt); - if (UFE.isValid()) { // Already have an entry with this inode, return it. + if (ReusingEntry) { // Already have an entry with this inode, return it.

[PATCH] D121375: [clang] NFC, move the utility function CompilerInvocation::setLangDefaults to LangOptions.h

dexonsmith added a comment. In D121375#3429023 , @sammccall wrote: > I only see one usage in-tree (and one more in clspv). And migration is very > easy. I think you should do it all in this commit. Nice, agreed, no need to split up.

[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 u

[PATCH] D123100: [Support/Hash functions] Change the `final()` and `result()` of the hashing functions to return an array of bytes

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

[PATCH] D123229: [clang][deps] Ensure deterministic file names on case-insensitive filesystems

dexonsmith accepted this revision. dexonsmith added a reviewer: benlangmuir. dexonsmith added a subscriber: benlangmuir. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Once we've removed the last use of `DirectoryEntry::getName()`, we should drop `DirectoryEnt

[PATCH] D123144: FileManager: std::map => BumpPtrAllocator + DenseMap of pointers

dexonsmith added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:318 FileEntryRef ReturnedRef(*NamedFileEnt); - if (UFE.isValid()) { // Already have an entry with this inode, return it. + if (ReusingEntry) { // Already have an entry with this inode, return it.

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

dexonsmith added a comment. > The ugly part here: > > - FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef > - I changed this to use a FileManager as a factory It's not clear *why* you changed this to use a FileManager as a factory. It seems unrelated to removing `FileEntry:

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

dexonsmith added reviewers: benlangmuir, bnbarham. dexonsmith added a comment. Adding a couple of reviewers that have been looking at FileManager issues with me recently; maybe they have a different perspective. (also, I neglected to say so explicitly before: removing `isValid()` SGTM; FileMana

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

dexonsmith added inline comments. Comment at: clang/lib/Basic/FileManager.cpp:462 UFE->Dir = &DirInfo->getDirEntry(); - UFE->UID = NextFileUID++; - UFE->IsValid = true; + UFE->UID = NextFileUID++; UFE->File.reset(); sammccall wrote: > kadircet wr

[PATCH] D129446: [clang][driver] Find Apple default SDK path

dexonsmith added a subscriber: arphaman. dexonsmith added a comment. I’m not at Apple anymore, but this is a long-standing platform decision that is intentional and seems unlikely to change. Note that `/usr/bin/clang` isn’t really clang, but a tool called `xcrun` which adds environment variables

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

dexonsmith added a comment. I haven’t reviewed the details of the patch and won’t have time to do so (at least for a while), but the description of the intended (more narrow) scope SGTM. With the scope limited to GCC directories this should be fine. There are cases where it’s safe to have misma

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

dexonsmith added a comment. In D126676#3687491 , @mstorsjo wrote: > In D126676#3687227 , @dexonsmith > wrote: > >> There are cases where it’s safe to have mismatched defines (e.g., if a >> define can be proven d

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

dexonsmith added a comment. In D105959#2882136 , @mehdi_amini wrote: > In D105959#2882099 , @bondhugula > wrote: > >> This is a really welcome change! Multiple registration issues were really an >> inconvenienc

[PATCH] D105959: Use ManagedStatic and lazy initialization of cl::opt in libSupport to make it free of global initializer

dexonsmith added a comment. In D105959#2883954 , @mehdi_amini wrote: > That's interesting! > > I'm not sure how to get there though: a complete solution should be able to > "degrade" to global constructor when the platform-specific logic isn't > availa

[PATCH] D106064: [clang][deps] Normalize paths in minimizing file system

dexonsmith added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:161-162 +const StringRef RawFilename) { + llvm::SmallString<256> Filename; + llvm::sys::path::native(RawFilename, Filename); + I'm a bit ner

[PATCH] D106146: [clang][deps] Separate filesystem caches for minimized and original files

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:165 - if (const CachedFileSystemEntry *Entry = getCachedEntry(Filename)) {

[PATCH] D104536: [clang][deps] Avoid minimizing PCH input files

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104536/new/ https://reviews.llvm.org/D104536 ___ cfe-commits mailing list cfe-

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

dexonsmith added a comment. In D105491#2882549 , @tmatheson wrote: > @dexonsmith I don't think they need upgraded. Most of the places I can see > referencing `!srcloc` are copying it around and will preserve the i32 type. > Cases which actually read the

[PATCH] D106064: [clang][deps] Normalize paths in minimizing file system

dexonsmith added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp:161-162 +const StringRef RawFilename) { + llvm::SmallString<256> Filename; + llvm::sys::path::native(RawFilename, Filename); + jansvoboda11

[PATCH] D106064: [clang][deps] Normalize paths in minimizing file system

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. In D106064#2887753 , @jansvoboda11 wrote: > With the call to `llvm::sys::path::native` scoped only to `IgnoredFiles`, > would this patch LGTY

[PATCH] D105958: [clang][darwin] add support for version remapping to the Darwin SDK Info class

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105958/new/ https://reviews.llvm.org/D105958 ___ cfe-commits mailing list cfe-

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM (one comment in the test), although it'd be good to get someone more involved in lib/CodeGen to take a quick look / sign off (ideally someone that knows the use case for `!srcloc`

[PATCH] D105491: [clang] Use i64 for the !srcloc metadata on asm IR nodes.

dexonsmith added a comment. In D105491#2893632 , @simon_tatham wrote: > In D105491#2891860 , @dexonsmith > wrote: > >> although it'd be good to get someone more involved in lib/CodeGen to take a >> quick look /

[PATCH] D105695: [clang][tooling] Accept Clang invocations with multiple jobs

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. Seeing the `-fembed-bitcode` case made me think of `-save-temps`. I think this will work since `-x cpp-output` should return false for `isSrcFile()`... but probably worth adding a test

[PATCH] D106787: [clang][driver] NFC: Move InputInfo.h from lib to include

dexonsmith accepted this revision. dexonsmith 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/D106787/new/ https://reviews.llvm.org/D106787 __

[PATCH] D106788: [clang][driver] NFC: Expose InputInfo in Job instead of plain filenames

dexonsmith accepted this revision. dexonsmith 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/D106788/new/ https://reviews.llvm.org/D106788 __

[PATCH] D106862: [clang][modules] Avoid creating partial FullSourceLoc for explicit modules imports

dexonsmith added a comment. In D106862#2913815 , @arphaman wrote: > I think this approach makes sense for now. It's unfortunate that the > constructor of FullSourceLoc is unable to validate this requirement, do you > know how many clients that you descr

[PATCH] D120465: [clang][deps] Disable implicit module maps

dexonsmith added inline comments. Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:269-273 + // However, some module maps loaded implicitly during the dependency scan can + // describe anti-dependencies. That happens when the current module is marked + /

[PATCH] D120474: [clang][deps] Remove '-fmodules-cache-path=' arguments

dexonsmith accepted this revision. dexonsmith 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/D120474/new/ https://reviews.llvm.org/D120474 __

[PATCH] D120465: [clang][deps] Disable implicit module maps

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with a suggested change for the text of the comment. Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:269-273 + // However, some module

[PATCH] D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API

dexonsmith added a comment. > Includes two test fixes (since chained mappings are no longer allowed) > and adds a new test for multiple overlays. Are we sure no one needs to support chained mappings? Has there been a ~~clang-dev~~ discourse discussion about it already? Just concerned that some

[PATCH] D121533: [clang][deps] Fix traversal of precompiled dependencies

dexonsmith added a comment. I'm not a big fan of moving to the recursive call. Creating a separate `StringMap` for each iteration seems awfully wasteful. I'd prefer fixing the iterative algorithm so that it's correct and easy to verify. This can be done with a stack that points at the stable po

[PATCH] D121295: [clang][deps] Modules don't contribute to search path usage

dexonsmith added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:305 // maps, searching for a module map that describes this module. - for (It = search_dir_begin(); It != search_dir_end(); ++It) { -if (It->isFramework()) { + for (DirectoryLookup Dir : search_

[PATCH] D121303: [clang][deps] Don't prune search paths used by dependencies

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:31 + [&](const serialization::ModuleFile *MF) -> void { +SearchPathUsage |=

[PATCH] D121549: Define ABI breaking class members correctly

dexonsmith accepted this revision. dexonsmith added a comment. LGTM too; thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121549/new/ https://reviews.llvm.org/D121549 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.

[PATCH] D121533: [clang][deps] Fix traversal of precompiled dependencies

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with another comment inline (up to you whether to do something with it). Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:92 + //

[PATCH] D121685: [clang][deps] NFC: Use range-based for loop instead of iterators

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM. (Not sure if you were waiting for review, but I think it's fine to push this kind of thing after checking the tests pass.) Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D121295: [clang][deps] Modules don't contribute to search path usage

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

[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

dexonsmith added a comment. In D121425#3384188 , @bnbarham wrote: > There's two failing tests with this change: > > - VFSFromYAMLTest.ReturnsExternalPathVFSHit > - VFSFromYAMLTest.ReturnsInternalPathVFSHit > > Apparently we allow relative paths in externa

[PATCH] D121425: [VFS] Add a new getVFSFromYAMLs API

dexonsmith added a comment. In D121425#3384479 , @bnbarham wrote: > In D121425#3384188 , @bnbarham > wrote: > >> There's two failing tests with this change: >> >> - VFSFromYAMLTest.ReturnsExternalPathVFSHit >> -

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added reviewers: dexonsmith, benlangmuir, bnbarham, arphaman, vsapsai. dexonsmith added a comment. I haven't had time yet to think through the implications of this. At first glance this seems fine/correct/great. The only case I know of where `./` means something is if you're running

[PATCH] D121997: [clang][driver] Fix compilation database dump with multiple architectures

dexonsmith added inline comments. Comment at: clang/test/Driver/compilation_database_multiarch.c:2 +// RUN: rm -rf %t && mkdir -p %t +// RUN: %clang -c %s -o %t/out -arch x86_64 -arch arm64 -MJ %t/compilation_database.json +// RUN: FileCheck --input-file=%t/compilation_database.

[PATCH] D121997: [clang][driver] Fix compilation database dump with multiple architectures

dexonsmith added inline comments. Comment at: clang/test/Driver/compilation_database_multiarch.c:5-6 + +// CHECK: { "directory": "{{.*}}", "file": "{{.*}}", "output": "{{.*}}", "arguments": [{{.*}} "--target=x86_64-apple-macosx12.0.0"]}, +// CHECK-NEXT: { "directory": "{{.*

[PATCH] D122030: Driver: Make macOS the default target OS for -arch arm64

dexonsmith created this revision. dexonsmith added reviewers: arphaman, ab, steven_wu. Herald added a subscriber: kristof.beyls. Herald added a project: All. dexonsmith requested review of this revision. Herald added a project: clang. This is a follow up to 565603cc94d79a8d0de6df840fd53714899fb890

[PATCH] D121997: [clang][driver] Fix compilation database dump with multiple architectures

dexonsmith added inline comments. Comment at: clang/test/Driver/compilation_database_multiarch.c:5-6 + +// CHECK: { "directory": "{{.*}}", "file": "{{.*}}", "output": "{{.*}}", "arguments": [{{.*}} "--target=x86_64-apple-macosx12.0.0"]}, +// CHECK-NEXT: { "directory": "{{.*

[PATCH] D121997: [clang][driver] Fix compilation database dump with multiple architectures

dexonsmith added a comment. In D121997#3393079 , @jansvoboda11 wrote: > Avoid buggy driver behavior I just remembered the "correct" way to specify a platform without an SDK if you're using `-arch`: `-mmacosx-version-min=12.0.0`. That's probably better

[PATCH] D121997: [clang][driver] Fix compilation database dump with multiple architectures

dexonsmith added inline comments. Comment at: clang/test/Driver/compilation_database_multiarch.c:2 +// RUN: rm -rf %t && mkdir -p %t +// RUN: %clang -c %s -o %t/out -arch x86_64 -arch arm64 -MJ %t/compilation_database.json +// RUN: FileCheck --input-file=%t/compilation_database.

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added reviewers: mstorsjo, rnk. dexonsmith added subscribers: mstorsjo, rnk. dexonsmith added a comment. FWIW, I quite like the clean up effect that this patch has (assuming we are able to convince ourselves that it's safe/correct for `FileManager` to do this). - Removing the dots fee

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added a comment. In D121733#3393552 , @bnbarham wrote: > In D121733#3393546 , @rnk wrote: > >> I've been somewhat afraid to touch this code because of symlinks. Is that >> something we need to worry ab

[PATCH] D121733: Clean pathnames in FileManager.

dexonsmith added a comment. In D121733#3393640 , @dexonsmith wrote: > In D121733#3393552 , @bnbarham > wrote: > >> In D121733#3393546 , @rnk wrote: >> >>> I've been some

[PATCH] D121997: [clang][driver] Fix compilation database dump with multiple architectures

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. The "Fix compilation database dump" changes LGTM, assuming you split out `-driver-only` and land it separately ahead of the rest of this. In D121997#3395562

[PATCH] D122237: [clang][lex] Fix failures with Microsoft header search rules

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

[PATCH] D121525: [clang][deps] Create lit substitution for deps-to-rsp

dexonsmith accepted this revision. dexonsmith 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/D121525/new/ https://reviews.llvm.org/D121525 __

[PATCH] D121812: [clang][deps] NFC: De-duplicate clang-cl tests

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

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/FileEntry.h:117 /// gcc5.3. Once that's no longer supported, change this back. -llvm::PointerUnion V; +llvm::PointerUnion V; These `const`-ness changes look independent and co

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/FileEntry.h:332-334 + FileEntry(); + FileEntry(const FileEntry &) = delete; + FileEntry &operator=(const FileEntry &) = delete; Aha, now I understand why you needed a factory in the FileEn

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

dexonsmith added a comment. (Seeing the other replies now!) In D123197#3433846 , @sammccall wrote: > I think it's worth making the test a bit uglier to achieve that, and by > contrast adding inheritance to FileManager would make it harder to understand.

[PATCH] D121375: [clang] NFC, move the utility function CompilerInvocation::setLangDefaults to LangOptions.h

dexonsmith added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:519-521 +void setLangDefaults(LangOptions &Opts, Language Lang, const llvm::Triple &T, + std::vector &Includes, + LangStandard::Kind LangStd); ---

[PATCH] D123197: Remove a few effectively-unused FileEntry APIs. NFC

dexonsmith accepted this revision. dexonsmith added a comment. LGTM; thanks for iterating on it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123197/new/ https://reviews.llvm.org/D123197 ___ cfe-commits

[PATCH] D121375: [clang] NFC, move the utility function CompilerInvocation::setLangDefaults to LangOptions.h

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM once @sammccall 's new comments are addressed! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121375/new/ https://reviews.llvm.org/D

[PATCH] D123468: [Driver] Simplify hasFlag pattern with addOptInFlag/addOptOutFlag helpers

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

[PATCH] D123398: [VFS] RedirectingFileSystem only replace path if not already mapped

dexonsmith added a comment. In D123398#3439946 , @bnbarham wrote: > Does `clang/test/VFS/external-names-multi-overlay.c` need to be formatted or > is it fine? It uses split-file so I'd really like to avoid the format here > (makes it pretty silly). See

[PATCH] D123398: [VFS] RedirectingFileSystem only replace path if not already mapped

dexonsmith accepted this revision. dexonsmith 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/D123398/new/ https://reviews.llvm.org/D123398 __

[PATCH] D123104: [Modules] Use looked-up filename when looking for module maps

dexonsmith added a comment. In D123104#3439261 , @bnbarham wrote: > Looks like there's more changes required for modulemap-collision.m to > actually pass. I'll try figure those out when I have the time. Capturing `clang -check` error since the build log

[PATCH] D123104: [Modules] Use looked-up filename when looking for module maps

dexonsmith added subscribers: jansvoboda11, vsapsai, Bigcheese. dexonsmith added a comment. In D123104#3443448 , @dexonsmith wrote: > I wonder if > https://github.com/apple/llvm-project/commit/67c70038bcc0d771e2e39c875ad7d69e329c7fc4 > could be related

[PATCH] D123574: [clang] NFCI: Use FileEntryRef in PPCallbacks::InclusionDirective

dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land. LGTM, with a couple of nitpicks. If you'd rather not improve the tests that's probably fine, since the patch doesn't make them any worse. Comment at: clang-tools-ext

[PATCH] D67678: PR17164: Change clang's default behavior from -flax-vector-conversions=all to -flax-vector-conversions=integer.

dexonsmith added a comment. In D67678#3446526 , @rsmith wrote: > @dexonsmith @arphaman What do we need to do to get this re-landed? (I'm sorry I missed your ping two years ago :(... I was on an extended leave at the time and never ended up catching up on

[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, > clang/test/SemaCXX/destruct

[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 by

[PATCH] D119714: [clang][lex] Remove `Preprocessor::GetCurDirLookup()`

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

[PATCH] D121873: [clang][extract-api] Add enum support

dexonsmith added inline comments. Comment at: clang/test/ExtractAPI/enum.c:5 +// RUN: %t/reference.output.json +// RUN: %clang -extract-api -target arm64-apple-macosx \ +// RUN: %t/input.h -o %t/output.json | FileCheck -allow-empty %s I just noticed these tests u

[PATCH] D124634: ExtractAPI: Use %clang_cc1 and -verify in enum.c

dexonsmith created this revision. dexonsmith added reviewers: zixuw, dang, ributzka. Herald added a project: All. dexonsmith requested review of this revision. Herald added a project: clang. Fix one test (enum.c) in ExtractAPI to use %clang_cc1 and -verify instead of calling the full driver and Fi

[PATCH] D121873: [clang][extract-api] Add enum support

dexonsmith added inline comments. Comment at: clang/test/ExtractAPI/enum.c:11 +// RUN: %t/output.json >> %t/output-normalized.json +// RUN: diff %t/reference.output.json %t/output-normalized.json + Diffing against reference output is usually not ideal: - It makes

[PATCH] D124635: Frontend: Delete output streams before closing CompilerInstance outputs

dexonsmith created this revision. dexonsmith added reviewers: akyrtzi, steven_wu, benlangmuir. Herald added a project: All. dexonsmith requested review of this revision. Herald added a project: clang. Delete the output streams coming from CompilerInstance::createOutputFile() and friends once write

[PATCH] D124635: Frontend: Delete output streams before closing CompilerInstance outputs

dexonsmith added a comment. In D124635#3481346 , @benlangmuir wrote: > LGTM, although I made a suggestion about the spelling. Thanks for the review! >> This fixes theoretical bugs related to proxy streams, which may have >> cleanups to run in their des

[PATCH] D124635: Frontend: Delete output streams before closing CompilerInstance outputs

This revision was automatically updated to reflect the committed changes. Closed by commit rG2d133867833f: Frontend: Delete output streams before closing CompilerInstance outputs (authored by dexonsmith). Changed prior to commit: https://reviews.llvm.org/D124635?vs=425893&id=425946#toc Reposit

[PATCH] D92160: [clang] Fix wrong FDs are used for files with same name in Tooling

dexonsmith added reviewers: benlangmuir, bnbarham, jansvoboda11. dexonsmith added subscribers: bnbarham, benlangmuir. dexonsmith added a comment. Adding @bnbarham, @jansvoboda11, and @benlangmuir, who have been looking at various FileManager issues recently. If you post a follow up I suggest keep

[PATCH] D124634: ExtractAPI: Use %clang_cc1 and -verify in enum.c

This revision was automatically updated to reflect the committed changes. Closed by commit rGc1e17c7dfedd: ExtractAPI: Use %clang_cc1 and -verify in enum.c (authored by dexonsmith). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124634/new/ https://r

[PATCH] D124816: [Tooling] use different FileManager for each CWD

dexonsmith added reviewers: benlangmuir, bnbarham, jansvoboda11. dexonsmith added a comment. Two high-level comments: - Making these functions `virtual` makes it harder to reason about possible implementations, but I don't think that's necessary for what you're doing here. Why not expose `ToolF

[PATCH] D124816: [Tooling] use different FileManager for each CWD

dexonsmith added a comment. In D124816#3494243 , @dexonsmith wrote: > - AFAICT, every call to `FileManager::setVirtualFileSystem()` is > fundamentally unsound unless the new FS is equivalent to the old one or the > filemanager hasn't been used yet. Mo

[PATCH] D124974: [clang] Include clang config.h in LangStandards.cpp

dexonsmith added a comment. Can we add a test for this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124974/new/ https://reviews.llvm.org/D124974 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D129226: [clang/mac] Make -mmacos-version-min the canonical spelling over -mmacosx-version-min

dexonsmith added a reviewer: akyrtzi. dexonsmith added a subscriber: akyrtzi. dexonsmith added a comment. LGTM too, but I’m not at Apple these days. @akyrtzi, can you help find someone appropriate to take a quick look since @arphaman seems busy? CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D124816: [LibTooling] use ToolFileManager to store file managers for each CWD

dexonsmith added a comment. This looks mostly correct to me, but perhaps pessimistic enough it'll regress performance unnecessarily. A few comments inline. Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:169-170 ScanInstance.getFrontendOpts(

[PATCH] D124974: [clang] Include clang config.h in LangStandards.cpp

dexonsmith added a comment. In D124974#3494374 , @sammccall wrote: > In D124974#3494279 , @dexonsmith > wrote: > >> Can we add a test for this? > > I think the problem with testing it is that this variable comes

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added subscribers: steven_wu, rjmccall, rnk, dexonsmith. dexonsmith added inline comments. Comment at: clang/test/Sema/warn-strict-prototypes.m:20 + // know it's a block when diagnosing. + void (^block2)(void) = ^void() { // expected-warning {{a function declaration

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added inline comments. Comment at: clang/test/Sema/warn-strict-prototypes.m:20 + // know it's a block when diagnosing. + void (^block2)(void) = ^void() { // expected-warning {{a function declaration without a prototype is deprecated in all versions of C}} };

[PATCH] D122895: [C89/C2x] Improve diagnostics around strict prototypes in C

dexonsmith added a comment. For additional context to my questions above, even though open source clang hasn't been using `-Wstrict-prototypes`, Xcode has had it on-by-default in new projects since sometime in 2017, with project modernizations to turn it on for old projects. Warning on block a

[PATCH] D124974: [clang] Include clang config.h in LangStandards.cpp

dexonsmith added a comment. In D124974#3504986 , @porglezomp wrote: > Ah, so it'd be a test that passes pretty trivially on any bot that doesn't > have a custom version of `CLANG_DEFAULT_STD_C` and `CLANG_DEFAULT_STD_CXX` > like the default config, but

[PATCH] D125488: [Preprocessor] Make the special lexing for dependency scanning a first-class feature of the `Preprocessor` and `Lexer`

dexonsmith added a comment. Awesome to see this! Looking forward to the next step (using this in normal preprocessing!). > after this change, we don't minimize sources and pass them in place of the > real sources Is there code in DepFS that can/should be deleted as part of this patch, or in a

<    1   2   3   4   5   6   7   8   9   10   >