[PATCH] D55363: [clangd] Expose FileStatus in LSP.

simark added a comment. In D55363#1329652 , @ilya-biryukov wrote: > Sorry if I missed any important design discussions here, but wanted to clear > up what information we are trying to convey to the user with the status > messages? > E.g. why seeing

[PATCH] D55250: [clangd] Enhance macro hover to see full definition

simark added inline comments. Comment at: clangd/XRefs.cpp:572 + + // Try to get the full definition, not just the name + SourceLocation StartLoc = Decl.Info->getDefinitionLoc(); hokein wrote: > if this is a complicated macro (like `AST_MATCHER`), do we still

[PATCH] D54077: [clangd] Implemented DraftFileSystem

simark added a comment. In https://reviews.llvm.org/D54077#1287153, @klimek wrote: > I'm in yet another camp: I carefully save when I have something that is > correct enough syntax, so I only want errors from with changes from the exact > file I'm editing and the rest of the files in saved

[PATCH] D53220: Remove possibility to change compile database path at runtime

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE344614: Remove possibility to change compile database path at runtime (authored by simark, committed by ). Changed prior to commit: https://reviews.llvm.org/D53220?vs=169828=169833#toc Repository:

[PATCH] D53220: Remove possibility to change compile database path at runtime

simark updated this revision to Diff 169828. simark marked 3 inline comments as done. simark added a comment. Address comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53220 Files: clangd/ClangdLSPServer.cpp clangd/Protocol.cpp clangd/Protocol.h

[PATCH] D53220: Remove possibility to change compile database path at runtime

simark marked 7 inline comments as done. simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:433 reparseOpenedFiles(); } sammccall wrote: > sammccall wrote: > > This isn't needed, the compilation database can only be set during > >

[PATCH] D53220: Remove possibility to change compile database path at runtime

simark marked an inline comment as done. simark added inline comments. Comment at: clangd/ClangdLSPServer.h:90 void reparseOpenedFiles(); + void applyConfiguration(const ClangdInitializationOptions ); void applyConfiguration(const ClangdConfigurationParamsChange );

[PATCH] D51725: Allow un-setting the compilation database path

simark added a comment. Thanks for your input. I have opened https://reviews.llvm.org/D53220, which removes that option. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D53220: Remove possibility to change compile database path at runtime

simark created this revision. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric, ilya-biryukov. This patch removes the possibility to change the compilation database path at runtime using the didChangeConfiguration request. Instead, it is suggested to use the setting on

[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol

simark added a comment. I just tried this, this looks very promising! It should help build our outline view in a much more robust way than we do currently. A nit for the final patch, I would suggest omitting the fields that are optional, like `children` (when the list is empty) and

[PATCH] D51725: Allow un-setting the compilation database path

simark added a comment. So I revisited this today. In the end, restarting clangd in our IDE works well. It should be merged anytime soon: https://github.com/theia-ide/theia/pull/3017 But I am wondering what to do with the feature that allows clangd to change compilation database path using

[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol

simark added a comment. Ohh awesome, I didn't know the LSP supported that. I'll try it it Theia when I have time. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D51725: Allow un-setting the compilation database path

simark added a comment. In https://reviews.llvm.org/D51725#1232748, @ilya-biryukov wrote: > If this setting exposed directly the users in Theia or is it something that > is exposed via a custom UI (e.g. choosing named build configs or something > similar)? It is through a custom UI, that we

[PATCH] D51725: Allow un-setting the compilation database path

simark added a comment. In https://reviews.llvm.org/D51725#1232092, @ilya-biryukov wrote: > Wow, this is getting somewhat complicated. > > Have you considered rerunning clangd whenever someone changes an option like > that? > Would that be much more complicated on your side? > > Not opposed to

[PATCH] D51725: Allow un-setting the compilation database path

simark added a comment. Not sure who should review this, please feel free to add anybody who would be appropriate. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D51725: Allow un-setting the compilation database path

simark updated this revision to Diff 164183. simark added a comment. Fix formatting. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51725 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/GlobalCompilationDatabase.cpp clangd/GlobalCompilationDatabase.h

[PATCH] D51725: Allow un-setting the compilation database path

simark created this revision. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric, ilya-biryukov. It is currently possible to tell clangd where to find the compile_commands.json file through the initializationOptions or the didChangeConfiguration message. However, it is

[PATCH] D51311: (WIP) Type hierarchy

simark added inline comments. Comment at: clangd/Protocol.h:891 + + std::vector Parents; + ilya-biryukov wrote: > What is the proposal to add children (i.e. overriden methods) to the > hierarchy? > This seems like a place where we might want to have multiple

[PATCH] D51311: (WIP) Type hierarchy

simark added inline comments. Comment at: clangd/Protocol.h:889 + // Does this node implement the method targeted by the request? + bool DeclaresMethod; + kadircet wrote: > I think comment and the name is in contradiction here, do you mean > DefinesMethod?

[PATCH] D51311: (WIP) Type hierarchy

simark created this revision. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric, ilya-biryukov. This is a work in progress patch to support an eventual "get type hierarchy" request that does not exist in the LSP today. I only plan to support getting parent types (i.e.

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

simark added inline comments. Comment at: lib/Basic/FileManager.cpp:319 - SmallString<128> RealPathName; - if (!FS->getRealPath(InterndFileName, RealPathName)) -UFE.RealPathName = RealPathName.str(); + if (UFE.File) { +if (auto Path = UFE.File->getName()) {

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

simark added inline comments. Comment at: lib/Basic/FileManager.cpp:319 - SmallString<128> RealPathName; - if (!FS->getRealPath(InterndFileName, RealPathName)) -UFE.RealPathName = RealPathName.str(); + if (UFE.File) { +if (auto Path = UFE.File->getName()) {

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

simark added inline comments. Comment at: lib/Basic/FileManager.cpp:319 - SmallString<128> RealPathName; - if (!FS->getRealPath(InterndFileName, RealPathName)) -UFE.RealPathName = RealPathName.str(); + if (UFE.File) { +if (auto Path = UFE.File->getName()) {

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

simark added inline comments. Comment at: lib/Basic/FileManager.cpp:319 - SmallString<128> RealPathName; - if (!FS->getRealPath(InterndFileName, RealPathName)) -UFE.RealPathName = RealPathName.str(); + if (UFE.File) { +if (auto Path = UFE.File->getName()) {

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark added inline comments. Comment at: clangd/SourceCode.h:65 /// Get the absolute file path of a given file entry. TextEdit toTextEdit(const FixItHint , const SourceManager , hokein wrote: > nit: this comment is not needed. Woops, merge mistake.

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE339483: [clangd] Avoid duplicates in findDefinitions response (authored by simark, committed by ). Changed prior to commit: https://reviews.llvm.org/D48687?vs=160175=160197#toc Repository: rCTE

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark updated this revision to Diff 160175. simark marked 4 inline comments as done. simark added a comment. Latest update. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48687 Files: clangd/FindSymbols.cpp clangd/SourceCode.cpp clangd/SourceCode.h clangd/XRefs.cpp

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark marked an inline comment as done. simark added a comment. In https://reviews.llvm.org/D48687#1195840, @hokein wrote: > Looks good with a few nits. Looks like you didn't update the patch correctly. > You have marked comments done, but your code doesn't get changed accordingly. > Please

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark marked 8 inline comments as done. simark added inline comments. Comment at: clangd/SourceCode.h:69 +llvm::Optional getRealPath(const FileEntry *F, +const SourceManager ); ilya-biryukov wrote: > This function looks

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark updated this revision to Diff 159972. simark marked an inline comment as done. simark added a comment. Replace RelPathPrefix == StringRef() with RelPathPrefix.empty() Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48687 Files: clangd/FindSymbols.cpp

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark marked 3 inline comments as done. simark added a comment. In https://reviews.llvm.org/D48687#1193470, @hokein wrote: > Sorry for the delay. I thought there was a dependent patch of this patch, but > it seems resolved, right? > > A rough round of review. Right, the patch this one

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark updated this revision to Diff 159768. simark added a comment. Formatting. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48687 Files: clangd/FindSymbols.cpp clangd/SourceCode.cpp clangd/SourceCode.h clangd/XRefs.cpp unittests/clangd/TestFS.cpp

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark updated this revision to Diff 159764. simark added a comment. Herald added a subscriber: arphaman. New version. I tried to share the code between this and SymbolCollector, but didn't find a good clean way. Do you have concrete suggestions on how to do this? Otherwise, would it be

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

This revision was automatically updated to reflect the committed changes. Closed by commit rC339063: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the… (authored by simark, committed by ). Changed prior to commit: https://reviews.llvm.org/D48903?vs=158625=159401#toc

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark added a comment. In https://reviews.llvm.org/D48903#1189056, @ilya-biryukov wrote: > I see, thanks for the explanation. > > LGTM for the update revision, given we have confirmation the tests pass on > Windows. Thanks, I'll push it, let's hope this time is the right time! Repository:

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark added a comment. In https://reviews.llvm.org/D48903#1188566, @malaperle wrote: > Both check-clang and check-clang-tools pass successfully for me on Windows > with the patch. Awesome, thanks! Repository: rC Clang https://reviews.llvm.org/D48903

[PATCH] D50255: Add test for changing build configuration

simark added a comment. In https://reviews.llvm.org/D50255#1187871, @arphaman wrote: > Thanks! This LGTM. Well, thanks for the hints! Repository: rL LLVM https://reviews.llvm.org/D50255 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D50255: Add test for changing build configuration

This revision was automatically updated to reflect the committed changes. Closed by commit rL338914: [clangd] Add test for changing build configuration (authored by simark, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D50255 Files:

[PATCH] D50255: Add test for changing build configuration

simark updated this revision to Diff 159050. simark added a comment. The tests now work on Windows, I added some path adjustment steps. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50255 Files: test/clangd/compile-commands-path-in-initialize.test

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark requested review of this revision. simark added a comment. In https://reviews.llvm.org/D48903#1187596, @ilya-biryukov wrote: > This revision got 'reopened' and is now in the list of accepted revision. > Should we close it again? It got reverted a second time because it was breaking a

[PATCH] D50255: Add test for changing build configuration

simark added a comment. This was not tested on windows yet. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50255 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50255: Add test for changing build configuration

simark created this revision. Herald added subscribers: cfe-commits, arphaman, jkorous, ioeric, ilya-biryukov. This patch adds tests for the two ways of changing build configuration (pointing to a particular compile_commands.json): - Through the workspace/didChangeConfiguration notification. -

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

simark added a comment. In https://reviews.llvm.org/D50154#1185605, @sammccall wrote: > Capitalizing sounds nice. +1 to just do it without an option. > > My favorite essay on this is > http://neugierig.org/software/blog/2018/07/options.html Agreed. Repository: rCTE Clang Tools Extra

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

simark added a comment. That's fine with me. I'll try it and time will tell if I like it or not :). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 158625. simark added a comment. Add -Wno-nonportable-include-path to test/PCH/case-insensitive-include.c Repository: rC Clang https://reviews.llvm.org/D48903 Files: lib/Basic/FileManager.cpp lib/Basic/VirtualFileSystem.cpp

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark added a subscriber: eric_niebler. simark added a comment. @eric_niebler, question for you: This patch causes clang to emit a `-Wnonportable-include-path` warning where it did not before. It affects the following test on Windows:

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

simark marked 2 inline comments as done. simark added a comment. Thanks, done. Repository: rL LLVM https://reviews.llvm.org/D49833 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

This revision was automatically updated to reflect the committed changes. Closed by commit rL338518: [clangd] Receive compilationDatabasePath in initialize request (authored by simark, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

simark updated this revision to Diff 158366. simark added a comment. Address Ilya's comment (add an indirection for initializationOptions). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49833 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/Protocol.cpp

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

simark added a comment. In https://reviews.llvm.org/D49833#1182615, @malaperle wrote: > @simark would you mind finishing this patch and committing it? I won't be > able to finish it given that I started it at a different company, etc. Yes, I'll take it over, thanks for getting it started.

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

This revision was automatically updated to reflect the committed changes. Closed by commit rC338057: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the… (authored by simark, committed by ). Changed prior to commit: https://reviews.llvm.org/D48903?vs=157467=157548#toc

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 157467. simark marked an inline comment as done. simark added a comment. I think this addresses all of Ilya's comments. Repository: rC Clang https://reviews.llvm.org/D48903 Files: lib/Basic/FileManager.cpp lib/Basic/VirtualFileSystem.cpp

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark marked 7 inline comments as done. simark added inline comments. Comment at: lib/Basic/VirtualFileSystem.cpp:475 InMemoryNodeKind Kind; + Status Stat; + ilya-biryukov wrote: > NIT: maybe keep the order of members the same to keep the patch more

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

simark added a comment. Thanks, that's simple and efficient. I'll update https://reviews.llvm.org/D49267 (to call `reparseOpenFiles` once again) once this is merged. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49783 ___

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 157287. simark added a comment. Fix tests on Windows Fix InMemoryFileSystem tests on Windows. The paths returned by the InMemoryFileSystem directory iterators in the tests mix posix and windows directory separators. THis is because we do queries with

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark reopened this revision. simark added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D48903#1175317, @ioeric wrote: > It would make it easier for your reviewers to look at the new changes if > you just reopen this patch and update the diff :) I

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark added a comment. I uploaded a new version of this patch here: https://reviews.llvm.org/D49804 Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49804: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name (take 2)

simark created this revision. simark added reviewers: malaperle, ilya-biryukov, bkramer. This is a new version of https://reviews.llvm.org/D48903, which has been reverted. @ioeric fixed the issues caused by this patch downstream, so he told me it was good to go again. I also fixed the test

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

simark added inline comments. Comment at: clangd/ClangdLSPServer.cpp:430 CDB.clear(); - -reparseOpenedFiles(); +compileCommandsChangePost(CCChangeData); } malaperle wrote: > ilya-biryukov wrote: > > Maybe keep the old logic of reparsing all open

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

simark added a comment. In https://reviews.llvm.org/D49267#1173266, @ilya-biryukov wrote: > The approach is not ideal, but may be a good middle ground before we figure > out how we approach file watching in clangd. Note that there are other things > that won't force the updates currently, e.g.

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

simark added a comment. In https://reviews.llvm.org/D49267#1171286, @ilya-biryukov wrote: > Thanks for putting up this change! It can be really annoying that clangd does > not pick up the compile commands that got updated. > > A few things of the top of my head on why we might want to punt on

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL337697: [clangd] Fix category in clangd-vscodes package.json (authored by simark, committed by ). Herald added a

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

This revision was automatically updated to reflect the committed changes. Closed by commit rC337284: [Tooling] Add operator== to CompileCommand (authored by simark, committed by ). Changed prior to commit: https://reviews.llvm.org/D49265?vs=155773=155883#toc Repository: rL LLVM

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

This revision was automatically updated to reflect the committed changes. Closed by commit rL337284: [Tooling] Add operator== to CompileCommand (authored by simark, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D49265 Files:

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

simark updated this revision to Diff 155773. simark added a comment. Add tests for both == and !=. I need to rebuild ~800 files (because I pulled llvm/clang), so I did not actually test it, but I'll do so before pushing tomorrow, of course. Repository: rC Clang

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

simark added a comment. In https://reviews.llvm.org/D49265#1164227, @dblaikie wrote: > In theory you'd need separate tests for op== and op!= returning false > (currently all the tests would pass if both implementations returned true in > all cases), but not the biggest deal. Good point, I'll

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

simark updated this revision to Diff 155753. simark added a comment. - Add test - Make operator== a function instead of method - Add operator!= (so I can use EXPECT_NE in the test, and because it may be useful in general) Repository: rC Clang https://reviews.llvm.org/D49265 Files:

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

simark added a comment. In https://reviews.llvm.org/D49265#1163740, @dblaikie wrote: > Any chance this can/should be unit tested? (also, in general (though might > not matter in this instance), symmetric operators like == should be > implemented as non-members (though they can still be friends

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

simark updated this revision to Diff 155284. simark added a comment. Remove unintended changes Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49267 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/GlobalCompilationDatabase.cpp

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

simark added a comment. Note, https://reviews.llvm.org/D49265 in clang is a prerequisite. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

simark created this revision. Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov. This patch adds support for watching for changes to compile_commands.json, and reparsing files if needed. The watching is done using the "workspace/didChangeWatchedFiles" notification,

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

simark created this revision. Herald added subscribers: cfe-commits, ioeric, ilya-biryukov. It does the obvious thing of comparing all fields. This will be needed for a clangd patch I have in the pipeline. Repository: rC Clang https://reviews.llvm.org/D49265 Files:

[PATCH] D49260: [clangd] JSONTracer: flush after writing event

simark created this revision. Herald added subscribers: cfe-commits, omtcyfz, jkorous, MaskRay, ioeric, ilya-biryukov. Let's say I use "CLANGD_TRACE=/tmp/clangd.json" and "tail -F /tmp/clangd.json", I'll often see unfinished lines, where the rest of the data is still in clangd's output buffer.

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

simark updated this revision to Diff 155232. simark added a comment. Oops. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49253 Files: clangd/clients/clangd-vscode/package.json Index: clangd/clients/clangd-vscode/package.json

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

simark updated this revision to Diff 155213. simark added a comment. - [clangd] JSONTracer: flush after writing event Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D49253 Files: clangd/Trace.cpp clangd/clients/clangd-vscode/package.json Index:

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

simark created this revision. Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov. When opening package.json, vscode shows: Use 'Programming Languages' instead Replacing "Languages" with this fixes it. Repository: rCTE Clang Tools Extra

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark added a comment. In https://reviews.llvm.org/D48903#1159303, @ioeric wrote: > For example, suppose you have header search directories `-I/path/to/include` > and `-I.` in your compile command. When preprocessor searches for an #include > "x.h", it will try to stat "/path/to/include/x.h"

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

simark added inline comments. Comment at: lib/Basic/FileManager.cpp:395 + SmallString<128> RealPathName; + if (!FS->getRealPath(InterndFileName, RealPathName)) +UFE->RealPathName = RealPathName.str(); ioeric wrote: > simark wrote: > > ioeric wrote: > > >

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

simark added a comment. In https://reviews.llvm.org/D49197#1159704, @ioeric wrote: > In https://reviews.llvm.org/D49197#1158972, @simark wrote: > > > Background: I'm trying to fix the cases why we receive a FileEntry without > > a real path computed in clangd, so we can avoid having to compute

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark updated this revision to Diff 155057. simark added a comment. This is an update of my work in progress. I haven't done any sharing with SymbolCollector, as it's not clear to me how to proceed with that. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48687 Files:

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

simark added inline comments. Comment at: lib/Basic/FileManager.cpp:395 + SmallString<128> RealPathName; + if (!FS->getRealPath(InterndFileName, RealPathName)) +UFE->RealPathName = RealPathName.str(); ioeric wrote: > It seems that at this point, we have

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark added a comment. In https://reviews.llvm.org/D48903#1159023, @ioeric wrote: > Would you mind reverting this patch for now so that we can come up with a > solution to address those use cases? > > Sorry again about missing the discussion earlier! Of course, feel free to revert if needed

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

simark added a comment. Background: I'm trying to fix the cases why we receive a FileEntry without a real path computed in clangd, so we can avoid having to compute it ourselves. Repository: rC Clang https://reviews.llvm.org/D49197 ___

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

simark added a comment. I'm not sure who should review this patch, please add anybody you think is qualified/responsible. Repository: rC Clang https://reviews.llvm.org/D49197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

simark updated this revision to Diff 155027. simark added a comment. Update commit message. Repository: rC Clang https://reviews.llvm.org/D49197 Files: lib/Basic/FileManager.cpp unittests/Basic/FileManagerTest.cpp Index: unittests/Basic/FileManagerTest.cpp

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

simark created this revision. Herald added a subscriber: cfe-commits. I noticed that when getVirtualFile is called for a file, subsequent calls to getFile will return a FileEntry without the RealPathName computed: const FileEntry *VFE = Mgr->getVirtualFile("/foo/../bar", ...); const

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

This revision was automatically updated to reflect the committed changes. Closed by commit rC336807: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the… (authored by simark, committed by ). Changed prior to commit: https://reviews.llvm.org/D48903?vs=154835=154995#toc

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

simark added a comment. Herald added a subscriber: omtcyfz. I took a look at `SymbolCollector`'s `toURI`, and I am not sure what to get from it. It seems like a lot of it could be replaced with a call to `FileSystem::getRealPath`. Repository: rCTE Clang Tools Extra

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 154835. simark added a comment. Bump SmallString size from 32 to 256 Repository: rC Clang https://reviews.llvm.org/D48903 Files: lib/Basic/FileManager.cpp lib/Basic/VirtualFileSystem.cpp unittests/Basic/VirtualFileSystemTest.cpp

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark marked an inline comment as done. simark added a comment. In https://reviews.llvm.org/D48903#1157330, @ilya-biryukov wrote: > LGTM if that does not introduce any regressions in clang and clang-tools. Thanks. I have seen no failures in `check-clang` and `check-clang-tools`, so I will

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 154782. simark marked 5 inline comments as done. simark added a comment. - Make InMemoryNode::Stat private again, add protected accessor. - Change condition formatting. Repository: rC Clang https://reviews.llvm.org/D48903 Files:

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 154662. simark added a comment. - Change InMemoryNode::getName to InMemoryNode::getFileName, to reduce the risk of mis-using it. Make the Stat field protected, make the subclasses' toString access it directly. Repository: rC Clang

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 154631. simark added a comment. - Use FileSystem::getRealPath in FileManager::getFile Repository: rC Clang https://reviews.llvm.org/D48903 Files: lib/Basic/FileManager.cpp lib/Basic/VirtualFileSystem.cpp unittests/Basic/VirtualFileSystemTest.cpp

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark marked an inline comment as done. simark added a comment. In https://reviews.llvm.org/D48903#1155403, @ilya-biryukov wrote: > In https://reviews.llvm.org/D48903#1154846, @simark wrote: > > > With the `InMemoryFileSystem`, this now returns a non-real path. The > > result is that we fill

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark marked 6 inline comments as done. simark added inline comments. Comment at: lib/Basic/VirtualFileSystem.cpp:488 + } + StringRef getName() const { return Stat.getName(); } InMemoryNodeKind getKind() const { return Kind; } ilya-biryukov wrote: > Given

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark added a comment. I found something fishy. There is this code in FileManager.cpp: if (UFE.File) if (auto RealPathName = UFE.File->getName()) UFE.RealPathName = *RealPathName; The real path is obtained from `UFE.File->getName()`. In the `RealFile` implementation, it returns

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 154422. simark marked an inline comment as done. simark added a comment. - Use StringRef in InMemoryNode::getStatus - Remove unused variable in TEST_F(InMemoryFileSystemTest, StatusName) Repository: rC Clang https://reviews.llvm.org/D48903 Files:

[PATCH] D49008: [clangd] Upgrade logging facilities with levels and formatv.

simark added a comment. In https://reviews.llvm.org/D49008#1154196, @sammccall wrote: > @simark - is it OK if I pick this up? Yes, totally fine. I didn't have time to follow up on my patch, and I think you did a great job here. I tested it, and I see that my main pain point was addressed.

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark updated this revision to Diff 154321. simark marked 4 inline comments as done. simark added a comment. - Add RequestedName to InMemoryNode::getStatus. - Also fix the directory_iterator code path. Repository: rC Clang https://reviews.llvm.org/D48903 Files:

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

simark marked 6 inline comments as done. simark added inline comments. Comment at: lib/Basic/VirtualFileSystem.cpp:516 + explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName) + : Node(Node), RequestedName (std::move (RequestedName)) + {}

  1   2   3   >