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

bnbarham created this revision. bnbarham added reviewers: dexonsmith, keith, JDevlieghere, vsapsai, sammccall. Herald added a subscriber: hiraditya. Herald added a project: All. bnbarham requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, c

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

bnbarham added a comment. `clang-apply-replacements/relative-paths.cpp` is failing, I haven't looked into it but my guess would be that it's from the `Status.getName() == Filename` -> `!Status.IsVFSMapped` change. That seems very odd to me. Comment at: clang/lib/Basic/FileMan

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

bnbarham added a comment. In D122549#3412064 , @dexonsmith wrote: > In D122549#3412021 , @bnbarham > wrote: > >> `clang-apply-replacements/relative-paths.cpp` is failing, I haven't looked >> into it but my gues

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

bnbarham updated this revision to Diff 418721. bnbarham edited the summary of this revision. bnbarham added a comment. Rename `IsVFSMapped` as suggested by Duncan. Update comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122549/new/ https://r

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

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

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

bnbarham 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: > dexonsmith wrote: > > An incremental patch you co

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

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

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

bnbarham updated this revision to Diff 418959. bnbarham added a comment. Keep using the redirection hack for the time being Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122549/new/ https://reviews.llvm.org/D122549 Files: clang/lib/Basic/FileMan

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3fda0edc51fd: [VFS] RedirectingFileSystem only replace path if not already mapped (authored by bnbarham). Repository: rG LLVM Github Monorepo CHA

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

bnbarham created this revision. bnbarham added a reviewer: dexonsmith. Herald added a subscriber: hiraditya. Herald added a project: All. bnbarham requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. This reverts commit 3fda0ed

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

bnbarham created this revision. bnbarham added a reviewer: dexonsmith. Herald added a project: All. bnbarham requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Prevent possible modulemap collisions by making sure to always use the looked-up fi

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

bnbarham added a comment. This broke crash reproducers in very specific circumstances, see https://reviews.llvm.org/D123104. I'll re-commit with adding `ExposesExternalVFSPath` instead of replacing `IsVFSMapped`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf65b0b5dcfeb: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped" (authored by bnbarham). Changed prior to commit: https:

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

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

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

bnbarham updated this revision to Diff 420678. bnbarham added a comment. Added a potential plan to remove the FileManager hacks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123104/new/ https://reviews.llvm.org/D123104 Files: clang/include/clan

[PATCH] D130934: [clang] Update code that assumes FileEntry::getName is absolute NFC

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. LGTM, thanks for doing this 🙇 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130934/new/ https://reviews.llvm.org/D130934 __

[PATCH] D130935: [clang] Only modify FileEntryRef names that are externally remapped

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Basic/FileManager.cpp:277-278 - if (Status.getName() == Filename) { -// The name matches. Set the FileEntry. + if (Status.getName() == F

[PATCH] D131004: [clang] Add FileEntryRef::getNameAsRequested()

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. Thanks 🙇 Comment at: clang/unittests/Basic/FileManagerTest.cpp:381 EXPECT_TRUE(F1->isSameRef(*F1Again)); - EXPECT_TRUE(F1->isSameRef(*F1Redirect)); + EXPECT_FALSE(F1

[PATCH] D131076: [clang][modules] Don't depend on sharing FileManager during module build

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/ClangScanDeps/modulemap-via-vfs.m:44 "case-sensitive": "false", + 'use-external-names': true, "roots": [ Nitpick: `'` v

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham updated this revision to Diff 358798. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105328/new/ https://reviews.llvm.org/D105328 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/include/clang/Basic/DiagnosticSerialization

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham marked 2 inline comments as done. bnbarham added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:1063 +<< ModuleName; +return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors; + } vsapsai wrote: > bnbarham

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham updated this revision to Diff 358826. bnbarham marked 2 inline comments as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105328/new/ https://reviews.llvm.org/D105328 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td clang/

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham marked an inline comment as done. bnbarham added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:2854 +bool recompileFinalized = +Result == OutOfDate && Capabilities & ARR_OutOfDate && +getModuleManager().getModuleCache(

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham marked an inline comment as done. bnbarham added a comment. I don't have commit access. @vsapsai or @akyrtzi would you mind committing this? The test failures seem unrelated to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105328/new/

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

bnbarham added a comment. In D95159#2897941 , @mibintc wrote: > Can you suggest how to find the problem? Thanks a lot. @mibintc the test is reusing the module cache directory and that's causing some issues between runs, sorry about that. You could add a

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

bnbarham added a comment. Yep, that would be perfect :). Thanks @mibintc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95159/new/ https://reviews.llvm.org/D95159 ___ cfe-commits mailing list cfe-commits@

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

bnbarham created this revision. bnbarham added reviewers: keith, dexonsmith, JDevlieghere, vsapsai. Herald added a subscriber: carlosgalvezp. Herald added a project: All. bnbarham requested review of this revision. Herald added projects: clang, LLDB, LLVM, clang-tools-extra. Herald added subscriber

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

bnbarham added inline comments. Comment at: llvm/include/llvm/Support/Error.h:1284 - StringRef getFileName() { return FileName; } + StringRef getFileName() const { return FileName; } Should this be in a change all by itself? Repository: rG LLVM Github M

[PATCH] D117730: [DNM][VFS] Do not overwrite the path when nesting RedirectingFileSystems

bnbarham abandoned this revision. bnbarham added a comment. Herald added a project: All. Rather than trying to fix nested RedirectingFileSystems, I've instead put up a bunch of patches to simplify RedirectingFileSystem. This does mean we can't "chain" remappings any more, but that seems like a g

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

bnbarham updated this revision to Diff 414718. bnbarham added a comment. Handle empty overlay file in clang tidy Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121426/new/ https://reviews.llvm.org/D121426 Files: clang-tools-extra/clang-tidy/tool/

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

bnbarham updated this revision to Diff 414751. bnbarham edited the summary of this revision. bnbarham added a comment. Update to single review dependency Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121426/new/ https://reviews.llvm.org/D121426 Fi

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

bnbarham added a comment. In D121426#3376442 , @dexonsmith wrote: >> 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

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

bnbarham marked an inline comment as done. bnbarham added inline comments. Comment at: llvm/include/llvm/Support/Error.h:1284 - StringRef getFileName() { return FileName; } + StringRef getFileName() const { return FileName; } dexonsmith wrote: > bnbarham wr

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

bnbarham updated this revision to Diff 415615. bnbarham edited the summary of this revision. bnbarham added a comment. Herald added subscribers: cfe-commits, lldb-commits, carlosgalvezp. Herald added projects: clang, LLDB, clang-tools-extra. Re-order to be before D121424

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

bnbarham marked an inline comment as done. bnbarham added a comment. Merged into D121425 instead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121426/new/ https://reviews.llvm.org/D121426 ___

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

bnbarham added a comment. There's two failing tests with this change: - VFSFromYAMLTest.ReturnsExternalPathVFSHit - VFSFromYAMLTest.ReturnsInternalPathVFSHit Apparently we allow relative paths in external-contents *without* specifying `overlay-relative: true`. In this case the relative paths ar

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

bnbarham 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 external-

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

bnbarham added a comment. In D121425#3384492 , @dexonsmith wrote: > Can you be more detailed about the overall state at the time of `cat`, which > causes it to fail? Sure. I think there's also some confusion with names here but I'm generally trying to

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

bnbarham planned changes to this revision. bnbarham added a comment. Blocked on the dependent Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121425/new/ https://reviews.llvm.org/D121425 ___ cfe-commits ma

[PATCH] D121733: Clean pathnames in FileManager.

bnbarham added a comment. In D121733#3392931 , @dexonsmith wrote: > However, FileManager changes sometimes have odd side effects... and it's > possible that somewhere in clang relies on having `FileManager::getFileRef()` > return precisely the same pat

[PATCH] D121733: Clean pathnames in FileManager.

bnbarham added a comment. In D121733#3392968 , @ppluzhnikov wrote: >> There's also some others where I wouldn't expect them to be failing in this >> patch, eg. the ones from `/` -> `{{[/\\]}}`. > > These are failing because `remove_dots` (un-intuitively

[PATCH] D121733: Clean pathnames in FileManager.

bnbarham added a comment. In D121733#3393546 , @rnk wrote: > I've been somewhat afraid to touch this code because of symlinks. Is that > something we need to worry about? Consider this path: root/symlink/../foo.h. > remove_dots will turn this into root/

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

bnbarham created this revision. bnbarham added reviewers: dexonsmith, keith, JDevlieghere, vsapsai, sammccall. Herald added a subscriber: hiraditya. Herald added a project: All. bnbarham requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, c

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

bnbarham abandoned this revision. bnbarham added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123104/ne

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

bnbarham added a comment. 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). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGfe2478d44e4f: [VFS] RedirectingFileSystem only replace path if not already mapped (authored by bnbarham). Repository: rG LLVM Github Monorepo CHA

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

bnbarham accepted this revision. bnbarham added a comment. Thanks Jan :)! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123574/new/ https://reviews.llvm.org/D123574 ___ cfe-commits mailing list cfe-commi

[PATCH] D123767: [clang][parse] NFCI: Use FileEntryRef in Parser::ParseModuleImport()

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. Thanks for doing all these Jan! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123767/new/ https://reviews.llvm.org/D123767

[PATCH] D123771: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::load*()

bnbarham added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:339 // Search for a module map file in this directory. -if (loadModuleMapFile(Dir.getDir(), IsSystem, +if (loadModuleMapFile(*Dir.getDirRef(), IsSystem, /*IsFramewor

[PATCH] D123771: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::load*()

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Lex/HeaderSearch.cpp:1583 +::getTopFrameworkDir(FileMgr, FrameworkName, SubmodulePath); +assert(TopFrameworkDir && "Could not find t

[PATCH] D123854: [clang][lex] NFCI: Use DirectoryEntryRef in FrameworkCacheEntry

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Lex/HeaderSearch.cpp:584 // If it is known and in some other directory, fail. - if (CacheEntry.Directory && CacheEntry.Directory != getFrame

[PATCH] D123856: [clang][lex] NFCI: Use FileEntryRef in ModuleMap::diagnoseHeaderInclusion()

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. Shadowing of `FE` almost tripped me up there 😅 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123856/new/ https://reviews.llvm.org/D123856 _

[PATCH] D124288: [Index] Add a USR and symbol kind for UnresolvedUsingIfExists

bnbarham created this revision. bnbarham added reviewers: benlangmuir, arphaman, jansvoboda11. Herald added a project: All. bnbarham requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. `UnresolvedUsingIfExists` has existed for a long time now,

[PATCH] D124288: [Index] Add a USR and symbol kind for UnresolvedUsingIfExists

bnbarham added inline comments. Comment at: clang/test/Index/using_if_exists.cpp:9 +// CHECK: [[@LINE-1]]:11 | using/C++ | foo | c:@UD@foo | | Decl | rel: 0 +// CHECK: [[@LINE-2]]:11 | using/using-unresolved/C++ | foo | c:using_if_exists.cpp@UUIE@foo | | Ref | rel: 0

[PATCH] D124288: [Index] Add a USR and symbol kind for UnresolvedUsingIfExists

bnbarham updated this revision to Diff 424621. bnbarham added a comment. After speaking with Ben, we decided it makes more sense to just remove the reference entirely. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124288/new/ https://reviews.llvm.

[PATCH] D124288: [Index] Remove reference to `UnresolvedUsingIfExists`

bnbarham updated this revision to Diff 424622. bnbarham retitled this revision from "[Index] Add a USR and symbol kind for UnresolvedUsingIfExists" to "[Index] Remove reference to `UnresolvedUsingIfExists`". bnbarham edited the summary of this revision. bnbarham added a comment. Update title/mes

[PATCH] D124288: [Index] Remove reference to `UnresolvedUsingIfExists`

bnbarham updated this revision to Diff 424631. bnbarham added a comment. Remove line for check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124288/new/ https://reviews.llvm.org/D124288 Files: clang/lib/Index/IndexDecl.cpp clang/test/Index/us

[PATCH] D124288: [Index] Remove reference to `UnresolvedUsingIfExists`

This revision was automatically updated to reflect the committed changes. Closed by commit rG089b6efefc3d: [Index] Remove reference to `UnresolvedUsingIfExists` (authored by bnbarham). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124288/new/ https:

[PATCH] D91580: [Frontend] Add flag to allow PCM generation despite compiler errors

bnbarham created this revision. bnbarham added a reviewer: akyrtzi. Herald added subscribers: cfe-commits, dang, arphaman. Herald added a project: clang. bnbarham requested review of this revision. As with precompiled headers, it's useful for indexers to be able to continue through compiler errors

[PATCH] D91580: [Frontend] Add flag to allow PCM generation despite compiler errors

bnbarham updated this revision to Diff 305622. bnbarham added a comment. Noticed I had left in the `-fdisable-module-hash` flags in the test, removed now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91580/new/ https://reviews.llvm.org/D91580 Files: clang/include/clang/Driver/Optio

[PATCH] D91580: [Frontend] Add flag to allow PCM generation despite compiler errors

bnbarham updated this revision to Diff 305914. bnbarham added a comment. Have allow-pcm also set allow-pch + test to make sure that works. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91580/new/ https://reviews.llvm.org/D91580 Files: clang/include/clang/Driver/Options.td clang/inc

[PATCH] D89024: [AST] Fix crashes caused by redeclarations in hidden prototypes

bnbarham created this revision. bnbarham added a reviewer: akyrtzi. Herald added subscribers: cfe-commits, arphaman, dexonsmith. Herald added a project: clang. bnbarham requested review of this revision. ObjCContainerDecl.getMethod returns a nullptr by default when the container is a hidden protot

[PATCH] D89024: [AST] Fix crashes caused by redeclarations in hidden prototypes

bnbarham updated this revision to Diff 296859. bnbarham added a comment. Ran clang-format again CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89024/new/ https://reviews.llvm.org/D89024 Files: clang/lib/AST/DeclObjC.cpp clang/test/Index/Inputs/hidden-redecls-sub.h clang/test/Index

[PATCH] D89024: [AST] Fix crashes caused by redeclarations in hidden prototypes

bnbarham updated this revision to Diff 296861. bnbarham added a comment. Update test to include explicit OSX target triple CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89024/new/ https://reviews.llvm.org/D89024 Files: clang/lib/AST/DeclObjC.cpp clang/test/Index/Inputs/hidden-redec

[PATCH] D89024: [AST] Fix crashes caused by redeclarations in hidden prototypes

bnbarham updated this revision to Diff 297098. bnbarham added a comment. Removed platform-specific path separator CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89024/new/ https://reviews.llvm.org/D89024 Files: clang/lib/AST/DeclObjC.cpp clang/test/Index/Inputs/hidden-redecls-sub.h

[PATCH] D95159: [ASTReader] Allow controlling separately whether validation should be disabled for a PCH vs a module file

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. LGTM, just the one minor comment Comment at: clang/lib/Serialization/ASTReader.cpp:2221- + // validation for the PCH and the modules it loads. + ModuleKind K = Curr

[PATCH] D95989: [ASTReader] Always rebuild a cached module that has errors

bnbarham created this revision. bnbarham added a reviewer: akyrtzi. bnbarham requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. A module in the cache with an error should just be a cache miss. If allowing errors (with -fallow-pcm-with-compiler

[PATCH] D95989: [ASTReader] Always rebuild a cached module that has errors

bnbarham updated this revision to Diff 321305. bnbarham added a comment. Added a couple semi-colons to the test file CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95989/new/ https://reviews.llvm.org/D95989 Files: clang/lib/Serialization/ASTReader.cpp clang/test/Modules/Inputs/error

[PATCH] D95989: [ASTReader] Always rebuild a cached module that has errors

bnbarham updated this revision to Diff 321308. bnbarham set the repository for this revision to rG LLVM Github Monorepo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95989/new/ https://reviews.llvm.org/D95989 Files: clang/lib/Serialization/ASTRea

[PATCH] D96246: Make sure a module file with errors produced via '-fallow-pcm-with-compiler-errors' can be loaded when using implicit modules

bnbarham accepted this revision. bnbarham 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/D96246/new/ https://reviews.llvm.org/D96246 _

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

bnbarham created this revision. bnbarham added reviewers: vsapsai, dexonsmith. Herald added a subscriber: kristof.beyls. bnbarham requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Reading modules first reads each control block in the chain an

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

bnbarham added a comment. I have another change to update the post-control-block functions to llvm::Error instead as well to hopefully make the distinction more clear as well. Let me know if you think it belongs in this PR, otherwise I'll open another once this one is in. Repository: rG LLV

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

bnbarham added inline comments. Comment at: clang/test/VFS/umbrella-mismatch.m:4 - -// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify -// RUN: %clang_c

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

bnbarham updated this revision to Diff 365619. bnbarham added a comment. Removed the now unused UsesFoo.framework in the tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 Files: clang/lib/Serializat

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

bnbarham updated this revision to Diff 366152. bnbarham marked an inline comment as done. bnbarham edited the summary of this revision. bnbarham added a comment. Changed to keep setting the umbrella header/directory with a FIXME that it currently doesn't work for framework modules. Repository:

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

bnbarham updated this revision to Diff 366154. bnbarham added a comment. Forgot to add the braces back in for the case statements Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 Files: clang/lib/Seriali

[PATCH] D107690: [Modules] Do not remove failed modules after the control block phase

bnbarham updated this revision to Diff 366378. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107690/new/ https://reviews.llvm.org/D107690 Files: clang/lib/Serialization/ASTReader.cpp clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h cl

[PATCH] D108268: [Modules] Change result of reading AST block to llvm::Error instead

bnbarham created this revision. bnbarham added reviewers: vsapsai, dexonsmith. bnbarham requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Reading the AST block can never fail with a recoverable error as modules cannot be removed during this p

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham created this revision. bnbarham added reviewers: akyrtzi, arphaman. Herald added a subscriber: mgorny. bnbarham requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. It was possible to re-add a module to a shared in-memory module cache w

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham added a comment. In D105328#2861028 , @vsapsai wrote: > This problem shouldn't happen based on module-level name hashing but for some > reason it is happening. If I'm not mistaken, InMemoryModuleCache uses .pcm > file name as the key. But that

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham updated this revision to Diff 356861. bnbarham added a comment. Updated the test comment with the actual cause (ie. `Top` being recompiled and inserted, not `M`). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105328/new/ https://reviews.l

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham added inline comments. Comment at: clang/unittests/Serialization/ModuleCacheTest.cpp:125-126 + ASSERT_TRUE(Invocation2); + CompilerInstance Instance2(Instance.getPCHContainerOperations(), + &Instance.getModuleCache()); + Instance2.setDiagno

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:1063 +<< ModuleName; +return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors; + } vsapsai wrote: > Can we get in infinite loop with `AllowPCMWithCompil

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham marked 5 inline comments as done. bnbarham added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:1063 +<< ModuleName; +return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors; + } bnbarham wrote: > vsapsai

[PATCH] D105328: [Frontend] Only compile modules if not already finalized

bnbarham updated this revision to Diff 358465. bnbarham edited the summary of this revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105328/new/ https://reviews.llvm.org/D105328 Files: clang/include/clang/Basic/DiagnosticCommonKinds.td cla

[PATCH] D113473: [clang][deps] Don't emit `-fmodule-map-file=`

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. That was quick! Seems reasonable to me -hopefully there's not some strange case where this wouldn't work, will be nice to avoid the parsing time (which actually isn't insignificant). Rep

[PATCH] D135634: [clang][modules] Serialize VFS overlay paths into PCMs

bnbarham added a comment. In D135634#3866353 , @benlangmuir wrote: > I think we should deduplicate the vfs overlays if the same ivfsoverlay is > specified in both the pcm and the command-line. > > @bnbarham any concern about overlay vs chaining behaviou

[PATCH] D135634: [clang][modules] Serialize VFS overlay paths into PCMs

bnbarham added a comment. In D135634#3866704 , @jansvoboda11 wrote: > In D135634#3866353 , @benlangmuir > wrote: > >> I think we should deduplicate the vfs overlays if the same ivfsoverlay is >> specified in bo

[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

bnbarham added a comment. Mostly just skimmed so far, will hopefully have some time to look at this more tomorrow. Out of interest, do you have any performance numbers using this change? I assume this mainly impacts implicit modules (since I suspect we'd also be opening the file as well anyway)

[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

bnbarham added a comment. > You're correct that this overhead has been measured on implicit module > builds. As I mentioned in the commit message this saves over 20% of the > overall built time in some cases. This time is split between module > validation (which could be skipped) and HeaderSear

[PATCH] D136844: [libclang] Expose completion result kind in `CXCompletionResult`

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. Thanks Egor! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136844/new/ https://reviews.llvm.org/D136844 ___

[PATCH] D134249: [modules] Fix error "malformed or corrupted AST file: 'SourceLocation remap refers to unknown module...'".

bnbarham accepted this revision. bnbarham added a comment. LGTM. > When we try to load such outdated .pcm file, we shouldn't change any already > loaded and processed modules. Or to put another way - we can't remove these modules because they may already have references... anywhere. Reposito

[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

bnbarham accepted this revision. bnbarham added inline comments. Comment at: clang/include/clang/Basic/SourceManager.h:146-155 /// References the file which the contents were actually loaded from. /// /// Can be different from 'Entry' if we overridden the contents of one

[PATCH] D115232: [clangd] Indexing of standard library

bnbarham added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:1008-1010 + // TUScheduler is the only thing that starts background indexing work. + if (IndexTasks && !IndexTasks->wait(timeoutSeconds(TimeoutSeconds))) +return false; @s

[PATCH] D115232: [clangd] Indexing of standard library

bnbarham added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:1008-1010 + // TUScheduler is the only thing that starts background indexing work. + if (IndexTasks && !IndexTasks->wait(timeoutSeconds(TimeoutSeconds))) +return false; sa

[PATCH] D115232: [clangd] Indexing of standard library

bnbarham added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:1008-1010 + // TUScheduler is the only thing that starts background indexing work. + if (IndexTasks && !IndexTasks->wait(timeoutSeconds(TimeoutSeconds))) +return false; sa

[PATCH] D141324: [clang] extend external_source_symbol attribute with the USR clause

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

[PATCH] D137304: [clang] Store filename per include instead of mutating filename

bnbarham added inline comments. Comment at: clang/include/clang/Basic/SourceManager.h:147-163 /// Indicates whether the buffer itself was provided to override /// the actual file contents. /// /// When true, the original entry may be a virtual file that does not //

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

bnbarham added a subscriber: dexonsmith. bnbarham added a comment. This seems reasonable to me in general. @dexonsmith in case you have any thoughts. Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4 + 'case-sensitive': false, + 'overlay-relative': true, + 'roo

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

bnbarham added inline comments. Comment at: clang/test/VFS/Inputs/root-relative-overlay.yaml:4 + 'case-sensitive': false, + 'overlay-relative': true, + 'root-relative': 'yaml-dir', bnbarham wrote: > I'd prefer a test without `overlay-relative` set to make it c

  1   2   >