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

[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 &Path) const; Should be private IMO Comment a

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

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

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

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

[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] 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. - CI.getPreprocessor().setMainFileDir

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

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

[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 }; + +/// IS_DOX

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

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

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

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

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

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

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

[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/lib/Support/VirtualFileSystem.cpp:1922 + EC = FS->makeAbsolute(FullPath, Name); + Name = canonicalize(Name); +} else { Why the canonicalization? Comment at: llvm/lib/Sup

[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 clang/test/Index/

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

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

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

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

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

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

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

[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 &HeaderInfo = *AST->HeaderInfo; - unsigned Counter; > I assume it's optional and ReadAST does not have to set the counter on > succ

[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 https://rev

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

bnbarham created this revision. Herald added subscribers: dexonsmith, hiraditya. bnbarham requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. 86e2af8043c7728710a711b623f27425801a36c3

[PATCH] D113832: reland: [VFS] Use original path when falling back to external FS

bnbarham added a comment. @keith This patch causes some issues when nesting/chaining `RedirectingFileSystems`. I have a few tests in https://reviews.llvm.org/D117730 for more details. Feedback on that PR would be appreciated, I'm not sure if it's the direction we want to go or not. Repositor

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

bnbarham added a comment. Also have bug up at https://github.com/llvm/llvm-project/issues/53306. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117730/new/ https://reviews.llvm.org/D117730 ___ cfe-commits

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

bnbarham added inline comments. Comment at: clang/test/VFS/directory.c:2 // RUN: rm -rf %t -// RUN: mkdir -p %t/Underlying -// RUN: mkdir -p %t/Overlay -// RUN: mkdir -p %t/Middle -// RUN: echo '// B.h in Underlying' > %t/Underlying/B.h -// RUN: echo '#ifdef NESTED' >> %t/Underl

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

bnbarham updated this revision to Diff 402130. bnbarham added a comment. Modified `directory.c` to make the test difference clearer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117730/new/ https://reviews.llvm.org/D117730 Files: clang/test/VFS

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

bnbarham added inline comments. Comment at: clang/test/VFS/directory.c:2 // RUN: rm -rf %t -// RUN: mkdir -p %t/Underlying -// RUN: mkdir -p %t/Overlay -// RUN: mkdir -p %t/Middle -// RUN: echo '// B.h in Underlying' > %t/Underlying/B.h -// RUN: echo '#ifdef NESTED' >> %t/Underl

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

bnbarham created this revision. bnbarham added reviewers: akyrtzi, dexonsmith, keith, JDevlieghere. Herald added a subscriber: hiraditya. bnbarham requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Extend "fallthrough" to all

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

bnbarham added a comment. Note: I am in no way attached to the name I came up with and would be very open to something better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117937/new/ https://reviews.llvm.org/D117937

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

bnbarham added a comment. In D117937#3268114 , @dexonsmith wrote: > The scenario you describe reminds me of the testcase in > https://reviews.llvm.org/D117730 ; can you explain if/how this patch relates > to that one? So a "fallback" is what we actual

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

bnbarham added a comment. Thanks for taking a look at that @keith, I appreciate it :)! I'll make those changes today hopefully. Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:575 +/// instead> +/// 'redirecting-with': keith wrot

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

bnbarham updated this revision to Diff 404194. bnbarham added a comment. Updated to address Keith's comments. Added an error for an invalid redirect kind as well as when both `redirect-with` and `fallthrough` are given. Also added tests for these cases. Repository: rG LLVM Github Monorepo C

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

bnbarham marked 5 inline comments as done. bnbarham added inline comments. Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:575 +/// instead> +/// 'redirecting-with': keith wrote: > bnbarham wrote: > > keith wrote: > > > I think `red

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

bnbarham marked an inline comment as done. bnbarham added a comment. Failures look unrelated Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117937/new/ https://reviews.llvm.org/D117937 ___ cfe-commits mai

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

bnbarham updated this revision to Diff 404999. bnbarham added a reviewer: nathawes. bnbarham added a comment. Noticed one of the unit tests wasn't actually testing the right thing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117937/new/ https://r

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

[PATCH] D100619: [ASTReader] Only mark module out of date if not already compiled

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. If a module contains errors (ie. it was built with -fallow-pcm-with-compiler-errors and had errors) and was from th

[PATCH] D100619: [ASTReader] Only mark module out of date if not already compiled

bnbarham added inline comments. Comment at: clang/test/Modules/Inputs/error/error.h:1 +#pragma mark mark + akyrtzi wrote: > Is this pragma relevant for the test? I'll double check but I believe assertions are hit regardless, but with assertions off the test does

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

bnbarham added a comment. In D108268#2958346 , @vsapsai wrote: > Not super happy with the complexity `DiagnosedError` introduces, want to > consider alternatives first. Ah yes, I wasn't happy with that either but couldn't come up with a decent alternat

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

bnbarham added a comment. In D108268#2958568 , @vsapsai wrote: > `DiagnosticError` looks like a good fit for the task at hand, so it is worth > to try it. Though I don't know if it would end up in the end convoluted or OK. Unless we also change `Diagnos

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

bnbarham added a subscriber: yaxunl. bnbarham added a comment. > ! In D108268#2961735 , @vsapsai > wrote: > This might be a stupid idea and a bridge too far but what if delayed > diagnostic was storing `PartialDiagnostic` and not three strings? This loo

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

bnbarham updated this revision to Diff 368781. bnbarham added a comment. Ended up just using DiagnosticError and reading the args from PartialDiagnostic. Still not really happy with this, but I think the alternative of allowing multiple in-flight diagnostics should be a separate PR regardless (

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

bnbarham updated this revision to Diff 368798. bnbarham marked 2 inline comments as done. bnbarham added a comment. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. Updated error handling and added the clang-format `#include` changes in. Repository: rG LLVM Github Monor

<    1   2