[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

bnbarham added a comment. In D148997#4561620 , @v.g.vassilev wrote: > So, in that case we should bring back the boolean flag for incremental > processing and keep the `IncrementalExtensions` LanguageOption separate. In > that case

[PATCH] D142560: Allow getRawCommentForDecl to find comments in macros

bnbarham added inline comments. Comment at: clang/test/Index/annotate-comments-objc.m:67-74 +#define DECLARE_ENUMS(name) \ + /** enumFromMacro IS_DOXYGEN_SINGLE */ \ + enum enumFromMacro { A }; \ + /** namedEnumFromMacro IS_DOXYGEN_SINGLE */ \ + enum name { B }; + +///

[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

bnbarham added a comment. In D148997#4561577 , @v.g.vassilev wrote: >> My other concern here is that it seems our use cases are somewhat different, >> eg. we wouldn't want any parsing differences and while I don't know why this >> is yet, the removal

[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

bnbarham added a comment. In D148997#4561211 , @v.g.vassilev wrote: https://github.com/llvm/llvm-project/blob/df6b35e329ebecad6dc3bfb83183e482eb7a0020/clang/lib/Parse/ParseExprCXX.cpp#L4070 >>> >>> That looks a bit obscure to me. Looks like we are

[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

bnbarham added a comment. In D148997#4561022 , @v.g.vassilev wrote: > I meant that I'd like to figure out if we could use the > `annot_repl_input_end` before considering a new flag. Oh sure, that's why I'm here :). Just trying to figure out what we

[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

bnbarham added a comment. In D148997#4559788 , @v.g.vassilev wrote: > I'd prefer to avoid adding a new flag. Is there a way to see how does the > diff looks like? You mean for a new flag? I don't have one prepared, but it would basically just be

[PATCH] D148997: [clang] Add a new annotation token: annot_repl_input_end

bnbarham added a comment. > Are there other users of incremental processing mode, other than the REPL / > IncrementalParser? It seems Swift's clang importer also uses incremental processing mode, I'm assuming to keep the `TUScope` and `CurLexer` alive after EOF. We also end up using the same

[PATCH] D154257: [clang] Fix leak in LoadFromCommandLineWorkingDirectory unit test

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa57bdc8fe687: [clang] Fix leak in LoadFromCommandLineWorkingDirectory unit test (authored by hamishknight, committed by bnbarham). Changed prior

[PATCH] D154134: [clang] Fix ASTUnit working directory handling

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG62e4c22c95a9: [clang] Fix ASTUnit working directory handling (authored by hamishknight, committed by bnbarham). Repository: rG LLVM Github

[PATCH] D127663: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::LookupFile

bnbarham accepted this revision. bnbarham added inline comments. Comment at: clang/lib/Frontend/FrontendAction.cpp:811 // Relative searches begin from CWD. - const DirectoryEntry *Dir = nullptr; - if (auto DirOrErr = CI.getFileManager().getDirectory(".")) -

[PATCH] D150492: [AST] Initialize local counter

bnbarham accepted this revision. bnbarham added inline comments. Comment at: clang/lib/Frontend/ASTUnit.cpp:825 HeaderSearch = *AST->HeaderInfo; - unsigned Counter; > I assume it's optional and ReadAST does not have to set the counter on > success.

[PATCH] D146656: [clang][ExtractAPI] Refactor ExtractAPIVisitor to make it more extensible

bnbarham added a comment. > I have decided against doing that, because we can't specify @LINE in the > c-index-test command line. FWIW you can do: // RUN: -pos=%(line+1):7 let bar = Bar() // CHECK ... [[@LINE-1]]:7 But I don't think this is particularly common, it's just how I

[PATCH] D146656: [clang][ExtractAPI] Refactor ExtractAPIVisitor to make it more extensible

bnbarham added inline comments. Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:170 bool operator()(SourceLocation Loc) { -// If the loc refers to a macro expansion we need to first get the file +// If the loc refersSourceLocationxpansion we need to first get

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

bnbarham added inline comments. Comment at: clang/lib/Index/USRGeneration.cpp:1032 + case TemplateArgument::UncommonValue: +// FIXME: Visit value. +break; bolshakov-a wrote: > bnbarham wrote: > > bolshakov-a wrote: > > > bnbarham wrote: > > > >

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

bnbarham added inline comments. Comment at: clang/lib/Index/USRGeneration.cpp:1032 + case TemplateArgument::UncommonValue: +// FIXME: Visit value. +break; bolshakov-a wrote: > bnbarham wrote: > > bolshakov-a wrote: > > > bnbarham wrote: > > > > akyrtzi

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

bnbarham added inline comments. Comment at: clang/lib/Index/USRGeneration.cpp:1032 + case TemplateArgument::UncommonValue: +// FIXME: Visit value. +break; bolshakov-a wrote: > bnbarham wrote: > > akyrtzi wrote: > > > erichkeane wrote: > > > >

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

bnbarham added inline comments. Comment at: clang/lib/Index/USRGeneration.cpp:1032 + case TemplateArgument::UncommonValue: +// FIXME: Visit value. +break; akyrtzi wrote: > erichkeane wrote: > > bolshakov-a wrote: > > > aaron.ballman wrote: > > > > Any

[PATCH] D142101: [clang] [extract-api] Don't crash for category in libclang APIs

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp:537-544 + generatePathComponents(Record, API, + [Lang, ](const

[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] 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;

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

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

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

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. LGTM. If you could put up a PR after to fix the use of `sys::fs::make_absolute` that would be appreciated . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138322: [Index] Add various missing USR generation

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG699ae92f0453: [Index] Add various missing USR generation (authored by bnbarham). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138322: [Index] Add various missing USR generation

bnbarham updated this revision to Diff 476614. bnbarham added a comment. Add missing --target Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138322/new/ https://reviews.llvm.org/D138322 Files: clang/lib/Index/USRGeneration.cpp

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

bnbarham added inline comments. Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1922 + EC = FS->makeAbsolute(FullPath, Name); + Name = canonicalize(Name); +} else { Why the canonicalization? Comment at:

[PATCH] D138322: [Index] Add various missing USR generation

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

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

bnbarham added inline comments. Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:1010 + std::error_code makeAbsolute(StringRef WorkingDir, + SmallVectorImpl ) const; Should be private IMO Comment at:

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

bnbarham added inline comments. Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:660 /// 'use-external-names': +/// 'root-relative': /// 'overlay-relative': haowei wrote: > bnbarham wrote: > > phosek wrote: > > > Could we make this just a

[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

[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, +

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

[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

[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

[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

[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

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

[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: `'`

[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)); +

[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() ==

[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] D127647: [clang][lex] NFCI: Use FileEntryRef in ModuleMap::{load,lookup}ModuleMap()

bnbarham added a comment. The failure here is likely due to the hack in FileManager::getFileRef: // FIXME: This hack ensures that `getDir()` will use the path that was // used to lookup this file, even if we found a file by different path // first. This is required in order to find a

[PATCH] D127663: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::LookupFile

bnbarham added a comment. Failing test seems to be because `.` is turned into the full path at some point. It's possible that this is because `getFileRef` returns the absolute path (see the massive FIXME there). If that's the case we could fix that by fixing `clang-apply-replacements` and then

[PATCH] D127663: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::LookupFile

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Frontend/FrontendAction.cpp:811 // Relative searches begin from CWD. - const DirectoryEntry *Dir = nullptr; - if (auto

[PATCH] D127660: [clang][lex] NFCI: Use DirectoryEntryRef in Preprocessor::MainFileDir

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Frontend/FrontendAction.cpp:507 // be resolved relative to the build directory of the module map file. -

[PATCH] D127654: [clang] NFCI: Use DirectoryEntryRef in Module::Directory

bnbarham added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:178 // Search for the header file within the module's home directory. - auto *Directory = M->Directory; + auto Directory = M->Directory; SmallString<128> FullPathName(Directory->getName());

[PATCH] D127654: [clang] NFCI: Use DirectoryEntryRef in Module::Directory

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Basic/Module.h:138 /// are found. - const DirectoryEntry *Directory = nullptr; +

[PATCH] D127651: [clang][lex] NFCI: Use DirectoryEntryRef in ModuleMap::parseModuleMapFile()

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/modularize/ModularizeUtilities.cpp:275 // Figure out the home directory for the module map file. - const DirectoryEntry *Dir =

[PATCH] D127648: [clang][lex] NFCI: Use DirectoryEntryRef in ModuleMap::inferFrameworkModule()

bnbarham accepted this revision. bnbarham added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Lex/ModuleMap.cpp:1028 // Look for an umbrella header. - SmallString<128> UmbrellaName = StringRef(FrameworkDir->getName()); +

[PATCH] D127647: [clang][lex] NFCI: Use FileEntryRef in ModuleMap::{load,lookup}ModuleMap()

bnbarham accepted this revision. bnbarham added a comment. This revision is now accepted and ready to land. Few small comments, LGTM otherwise. Comment at: clang/include/clang/Lex/HeaderSearch.h:627 /// Try to find a module map file in the given directory, returning ///

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

[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

[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

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

[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 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] 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] 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 !=

[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

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

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

[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

[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

[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

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

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

[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

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

[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

[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

[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

[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

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

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

[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

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

[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

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

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

[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

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

[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

[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

[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

[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

[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

[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 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 inline comments. Comment at: llvm/include/llvm/Support/Error.h:1284 - StringRef getFileName() { return FileName; } + StringRef getFileName() const { return FileName; } dexonsmith wrote: > bnbarham

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

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

[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

[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

[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

[PATCH] D117937: [VFS] Add a "redirecting-with" field to overlays

bnbarham added a comment. @keith I don't have commit access, would you be able to merge this if you're happy with it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117937/new/ https://reviews.llvm.org/D117937

  1   2   >