[PATCH] D71403: [clangd] Fix hover crashing on null types

ilya-biryukov added a comment. Wow, `ParenListExpr` is a really weird construct... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71403/new/ https://reviews.llvm.org/D71403 ___ cfe-commits mailing list

[PATCH] D71403: [clangd] Fix hover crashing on null types

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

[PATCH] D71403: [clangd] Fix hover crashing on null types

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:251 QualType T = E->getType(); - if (T->isFunctionType() || T->isFunctionPointerType() || + if (T.isNull() || T->isFunctionType() || T->isFunctionPointerType() || T->isFunctionReferenc

[PATCH] D71356: [Tooling/Syntax] Helpers to find spelled tokens touching a location.

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:261 + bool AcceptRight = Right != All.end() && !(Loc < Right->location()); + bool AcceptLeft = Right != All.begin() && !((Right - 1)->endLocation

[PATCH] D71356: [Tooling/Syntax] Helpers to find spelled tokens touching a location.

ilya-biryukov added inline comments. Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:270 + for (const syntax::Token &Tok : spelledTokensTouching(Loc, Tokens)) +if (Tok.kind() == tok::identifier) + return &Tok; kbobyrev wrote: > ilya-biryukov wrote: > >

[PATCH] D71356: [Tooling/Syntax] Helpers to find spelled tokens touching a location.

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:270 + for (const syntax::Token &Tok : spelledTokensTouching(Loc, Tokens)) +if (Tok.kind() == tok::identifier) + return &Tok;

[PATCH] D71406: [clangd] Add xref for macros to FileIndex.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/URI.h:115 +// the SourceManager. +URI toURI(llvm::StringRef Path, const SourceManager &SM, + const std::string &FallbackDir); This function does a very specialized form of path-to-

[PATCH] D71543: [clangd] Fix handling of inline/anon namespaces and names of deduced types in hover

ilya-biryukov added a comment. My biggest concern is that we seem to make output for template instantiation worse. There should be a way to stop showing anonymous namespace without introducing such regressions. Comment at: clang-tools-extra/clangd/Hover.cpp:353 /// Generate

[PATCH] D71544: [clangd] Improve printing of lambda names

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

[PATCH] D71455: [NFC] Fix typos in Clangd and Clang

ilya-biryukov added a comment. In addition to that, NFC is also often used as a way to tag commits that should not cause any problems. wrt to this change, I don't think anything was done wrong. It even went through review, although often we don't send reviews for NFC changes. Downstream project

[PATCH] D71586: [clangd] Add a test case that verifies -fimplicit-modules work in Clangd when building the AST

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. +1, please add a unit test instead. We prefer to keep the number of lit tests in clangd minimal. They're mostly testing the LSP layer and other things which are hard to

[PATCH] D71545: [clangd] Improve hover for auto on template instantiations

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:556 +// FIXME: Drop default arguments. +HI.Name = "Foo"; +HI.Kind = index::SymbolKind::Class; NIT: Maybe remove default argument

[PATCH] D71543: [clangd] Fix handling of inline/anon namespaces and names of deduced types in hover

ilya-biryukov added a comment. In D71543#1785842 , @kadircet wrote: > I've got D71545 to reduce that regression. D71545 seems to be pretty small, yet depends on this change. Maybe add i

[PATCH] D71543: [clangd] Fix handling of inline/anon namespaces and names of deduced types in hover

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:353 /// Generate a \p Hover object given the type \p T. HoverInfo getHoverContents(QualType T, const Decl *D, ASTContext &ASTCtx, + const SymbolIndex *Index) {

[PATCH] D71543: [clangd] Fix handling of inline/anon namespaces and names of deduced types in hover

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:353 /// Generate a \p Hover object given the type \p T. HoverInfo getHoverContents(QualType T, const Decl *D, ASTContext &ASTCtx, + const SymbolIndex *Index) {

[PATCH] D71543: [clangd] Fix handling of inline/anon namespaces and names of deduced types in hover

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:372 +T.print(OS, Policy); +OS.flush(); + } NIT: is flush redundant? I believe it's called in destructor Comment at: clang-tools-extra/clangd/Hover.cpp:

[PATCH] D71597: [clangd][NFC] Make use of TagDecl inside type for hover on auto

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:353 - if (D) { + if (const auto *D = T->getAsTagDecl()) { HI.Kind = index::getSymbolInfo(D).Kind; This might be a functional change in case of typedefs. Could you check

[PATCH] D71406: [clangd] Add xref for macros to FileIndex.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:86 +// Add macro references. +for (const auto &IDToRefs : MacroRefsToIndex->MacroRefs) { + for (const auto &Range : IDToRefs.second) { hokein wrote: > ilya-

[PATCH] D71598: [clangd] Filter implicit references from index while renaming

ilya-biryukov added a comment. If we go with the solution proposed by @hokein, it looks like using the current patch is an improvement of what we have now. One big issue with the adding a new ref kind/ref modifier is that it requires modifications to Kythe-based index implementation, something t

[PATCH] D71597: [clangd][NFC] Make use of TagDecl inside type for hover on auto

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

[PATCH] D71543: [clangd] Fix handling of inline/anon namespaces and names of deduced types in hover

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

[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:188 +// returns D. +const NamedDecl *getExplicitSpec(const NamedDecl *D) { + if (auto *CTSD = llvm::dyn_cast(D)) { What's the purpose of this function? I don't think its descript

[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:188 +// returns D. +const NamedDecl *getExplicitSpec(const NamedDecl *D) { + if (auto *CTSD = llvm::dyn_cast(D)) { kadircet wrote: > ilya-biryukov wrote: > > What's the purpose o

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

ilya-biryukov updated this revision to Diff 234359. ilya-biryukov marked 24 inline comments as done. ilya-biryukov added a comment. - Addressed most comments - Rebased onto HEAD Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64573/new/ https://revie

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/BuildTree.h:25 +/// Allows to create syntax trees from subtrees not backed by the source code. +class Factory { +public: gribozavr2 wrote: > Why a class to contain static function

[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:188 +// returns D. +const NamedDecl *getExplicitSpec(const NamedDecl *D) { + if (auto *CTSD = llvm::dyn_cast(D)) { kadircet wrote:

[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:191 + // attached to that. + if (auto *CTSD = llvm::dyn_cast(D)) { +if (!CTSD->isExplicitInstantiationOrSpecialization()) You might need to do the same for specializations o

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

ilya-biryukov updated this revision to Diff 234494. ilya-biryukov marked 6 inline comments as done. ilya-biryukov added a comment. - Fix a header guard - Make firstLeaf and lastLeaf methods inside Tree - Mark non-modifiable nodes with I (for immutable) Repository: rG LLVM Github Monorepo CHAN

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

ilya-biryukov marked an inline comment as not done. ilya-biryukov added inline comments. Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:57 + return nullptr; +} + gribozavr2 wrote: > Seems like these first/last helpers should be methods on `syntax::Node`

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

This revision was automatically updated to reflect the committed changes. Closed by commit rG1ad15046dcf6: [Syntax] Allow to mutate syntax trees (authored by ilya-biryukov). Changed prior to commit: https://reviews.llvm.org/D64573?vs=234494&id=234496#toc Repository: rG LLVM Github Monorepo

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

ilya-biryukov marked 3 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:528 +void test() { + HALF_IF HALF_IF_2 else {} +})cpp", ilya-biryukov wrote: > gribozavr2 wrote: > > Could you also do so

[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Hover.cpp:188 +// Returns the decl that should be used for querying comments, either from index +// or ast. +const NamedDecl *getDeclForComment(const NamedDecl *D) { NIT: AST ===

[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.h:194 +llvm::SmallVector +explicitReferenceTargets(ast_type_traits::DynTypedNode N, + DeclRelationSet Mask = {}); No need to fix this. The name could pro

[PATCH] D71652: [clangd] Replace shortenNamespace with getQualification

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/AST.cpp:305 + llvm::raw_string_ostream OS(Result); + if (auto *TD = QT->getAsTagDecl()) +OS << getQualification(CurContext.getParentASTContext(), &CurContext, TD, Why not use `explici

[PATCH] D71596: [clangd] Improve documentation for auto and implicit specs

ilya-biryukov accepted this revision. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:136 HI.Kind = index::SymbolKind::Cl

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

ilya-biryukov added a comment. Thanks for the fixes! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64573/new/ https://reviews.llvm.org/D64573 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov created this revision. ilya-biryukov added a reviewer: rsmith. Herald added a reviewer: martong. Herald added a reviewer: shafik. Herald added a project: clang. This changes introduces an enum to represent dependencies as a bitmask and extract common patterns from code that computes

[PATCH] D72508: [clangd] Support pseudo-obj expr, opaque values, and property references in findExplicitReferences()

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:794 +// Not clear why the source expression is skipped by default... +return RecursiveASTVisitor::TraverseStmt(OVE->getSourceExpr()); + } Should this be done by `Rec

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov updated this revision to Diff 240604. ilya-biryukov added a comment. - Use DependencyFlags for TemplateName and NestedNameSpecifier Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 Files: cla

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/DependencyFlags.h:17-28 +enum class DependencyFlags : uint8_t { + Type = 1, + Value = 2, + Instantiation = 4, + UnexpandedPack = 8, + + // Shorthands for

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov updated this revision to Diff 240958. ilya-biryukov marked an inline comment as done and an inline comment as not done. ilya-biryukov added a comment. - Use different types for different AST categories - Rename to getDependence Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D71920: [AST] Refactor propagation of dependency bits. NFC

ilya-biryukov added a comment. I've opted for duplicating the common flags across all the introduced enums (contains-unexpanded-pack, instantiation-dependent) , this is somewhat ugly, but everything else is even more complicated to use. Less enums would also be a good idea probably, see the rele

[PATCH] D73617: [clangd] Don't mmap source files on all platforms --> don't crash on git checkout

ilya-biryukov accepted this revision. ilya-biryukov 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/D73617/new/ https://reviews.llvm.org/D73617

[PATCH] D73638: [AST] Move dependence computations into a separate file

ilya-biryukov added a comment. @rsmith this does not pass through testing, I've messed up somewhere while moving the code. I'll find what went wrong and fix it tomorrow. Please tell if the approach itself LG. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D73723: [clangd][Hover] Handle uninstantiated default args

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/Hover.cpp:276 +// we only print the expression. +if (PVD->hasDefaultArg() && !PVD->hasUnparsedDefaultArg()) {

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov updated this revision to Diff 241706. ilya-biryukov added a comment. Herald added a subscriber: bmahjour. Rebase on top of refactored dependence propagation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65591/new/ https://reviews.llvm

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

ilya-biryukov added a comment. As soon as I add a new enum value, this starts crashing some tests (even if we don't ever set the flag or serialize it). Not sure what causes it and won't have time to figure it out before I leave. Sorry about that :( Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D69330: [AST] Add RecoveryExpr to retain expressions on semantic errors

ilya-biryukov updated this revision to Diff 241717. ilya-biryukov added a comment. Herald added a subscriber: bmahjour. - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69330/new/ https://reviews.llvm.org/D69330 Files: clang/include/clang/

[PATCH] D69330: [AST] Add RecoveryExpr to retain expressions on semantic errors

ilya-biryukov updated this revision to Diff 241718. ilya-biryukov added a comment. Fix compilation after rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69330/new/ https://reviews.llvm.org/D69330 Files: clang/include/clang/AST/ComputeDepend

[PATCH] D44673: Make positionToOffset return llvm::Expected

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. See the empty line NITS, though. Comment at: clangd/ClangdServer.cpp:199 +return End.takeError(); + + return formatCode(Code, File, {tooling::Range(*

[PATCH] D44673: Make positionToOffset return llvm::Expected

ilya-biryukov added inline comments. Comment at: clangd/SourceCode.h:41 /// Turn an offset in Code into a [line, column] pair. Position offsetToPosition(llvm::StringRef Code, size_t Offset); We should be consistent for all functions inside this fail. They may

[PATCH] D44673: Make positionToOffset return llvm::Expected

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:199 +return End.takeError(); + + return formatCode(Code, File, {tooling::Range(*Begin, *End - *Begin)}); simark wrote: > ilya-biryukov wrote: > > NIT: unnecessary empty line > In genera

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov added inline comments. Comment at: clangd/DraftStore.h:36 /// Replace contents of the draft for \p File with \p Contents. - void updateDraft(PathRef File, StringRef Contents); + void addDraft(PathRef File, StringRef Contents); + simark wrote: >

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov added inline comments. Comment at: unittests/clangd/DraftStoreTests.cpp:27 + +static int rangeLength(StringRef Code, const Range &Rng) { + size_t Start = positionToOffset(Code, Rng.start); ilya-biryukov wrote: > no need for `static`, since function

[PATCH] D44764: [clangd] Move GTest printers to separate file

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. I wonder if there's a mechanism to always include the printers when including `gtest.h`, but having a convention to always include them seems ok for now. Repository: rCT

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov added a comment. Thanks for addressing the comments. I have just one comment left, about the LSP versions and sanity-checking. Comment at: clangd/DraftStore.h:36 /// Replace contents of the draft for \p File with \p Contents. - void updateDraft(PathRef File,

[PATCH] D44764: [clangd] Move GTest printers to separate file

ilya-biryukov added subscribers: sammccall, hokein, bkramer. ilya-biryukov added a comment. In https://reviews.llvm.org/D44764#1045648, @simark wrote: > We could create a file `ClangdTesting.h" which includes everything tested > related (gtest, gmock, printers, syncapi, etc). The convention woul

[PATCH] D44787: Migrate dockerfiles to use multi-stage builds.

ilya-biryukov created this revision. ilya-biryukov added reviewers: mehdi_amini, klimek. Herald added a subscriber: llvm-commits. We previously emulated multi-staged builds using two dockerfiles, native support from Docker allows us to merge them into one, simplifying our scripts. For more detail

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov added a comment. Thanks again. This generally looks LGTM, just a last drop of small nits. Will approve as soon as they land. Comment at: clangd/DraftStore.cpp:75 + return llvm::make_error( + llvm::formatv("Range's end position (line={0}, character={

[PATCH] D44272: [clangd] Support incremental document syncing

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Comment at: unittests/clangd/DraftStoreTests.cpp:1 +//===-- DraftStoreTests.cpp - Clangd unit tests -*- C++ -*-===// +//

[PATCH] D44764: [clangd] Use a header for common inclusions for tests

ilya-biryukov added a comment. In https://reviews.llvm.org/D44764#1046766, @sammccall wrote: > You raise a good point. If we can get away with using `operator <<` everywhere, it is definitely the best approach. Then our rule of thumb is: never define `PrintTo` overload, define `operator <<` f

[PATCH] D44882: [clangd] Implementation of workspace/symbol request

ilya-biryukov added reviewers: sammccall, ilya-biryukov. ilya-biryukov added a subscriber: sammccall. ilya-biryukov added a comment. Adding @sammccall, he will definitely want to take a look at index-related changes. On a high level, the approach seems just right. A few questions immedieately tha

[PATCH] D44882: [clangd] Implementation of workspace/symbol request

ilya-biryukov added a comment. >>> - Current `fuzzyFind` implementation can only match qualifiers strictly >>> (i.e. st::vector won't match std::vector). Should we look into allowing >>> fuzzy matches for this feature? (not in this patch, rather in the long >>> term). >> >> I'm not sure, I'm

[PATCH] D44882: [clangd] Implementation of workspace/symbol request

ilya-biryukov added a comment. > I'm highly in favor of enabling fuzzy matching for `workspaceSymbols`. At lest for the name themselves. Non-fuzzy matching of qualifiers does not look that important. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44882 _

[PATCH] D44932: [CodeComplete] Fix completion in the middle of ident in ctor lists.

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, aaron.ballman, bkramer, sepavloff. The example that was broken before (^ designate completion points): class Foo { Foo() : fie^ld^() {} // no completions were provided here. int field; }; To fix it we don'

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

ilya-biryukov updated this revision to Diff 139926. ilya-biryukov added a comment. - Replace auto with explicit type Repository: rC Clang https://reviews.llvm.org/D44480 Files: lib/Sema/SemaDecl.cpp test/CodeCompletion/crash-skipped-bodies-template-inst.cpp Index: test/CodeCompletion/c

[PATCH] D44480: [Sema] Don't skip function bodies with 'auto' without trailing return type

ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. I hit that bug in practice a few days ago when playing around with a C++17 codebase. It makes completion totally unusable when functions with auto-deduced return type are used. @rsmith, any chance you could take a loo

[PATCH] D41537: Optionally add code completion results for arrow instead of dot

ilya-biryukov added a comment. Sorry for the late response, was on vacation. Comment at: include/clang/Sema/CodeCompleteConsumer.h:565 + /// \brief For this completion result correction is required. + Optional Corr = None; + yvvan wrote: > yvvan wrote: > > il

[PATCH] D45006: [Tooling] A CompilationDatabase wrapper that infers header commands.

ilya-biryukov added inline comments. Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:26 +// their compile commands. If getAllFilenames() is empty, no inference occurs. +// +//===--===// -

[PATCH] D45007: [clangd] Use compile-command interpolation to provide commands for header files.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Let's just always do it! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45007 ___ cfe-commits mailing list cfe-com

[PATCH] D45069: [clangd] synthesize fix message when the diagnostic doesn't provide one.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a small nit Comment at: clangd/Diagnostics.cpp:322 + StringRef Insert = FixIt.CodeToInsert; + if (!Invalid && Remove.size() + Insert.size()

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:412 +llvm::raw_string_ostream OS(Message); +OS << "method not found (" << Method << ")"; +replyError(ErrorCode::MethodNotFound, OS.str()); malaperle wrote: > ilya-biryukov wro

[PATCH] D44932: [CodeComplete] Fix completion in the middle of ident in ctor lists.

ilya-biryukov added a reviewer: arphaman. ilya-biryukov added a comment. +Alex, in case he might know someone who can review it. Repository: rC Clang https://reviews.llvm.org/D44932 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://l

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:412 +llvm::raw_string_ostream OS(Message); +OS << "method not found (" << Method << ")"; +replyError(ErrorCode::MethodNotFound, OS.str()); simark wrote: > simark wrote: > > il

[PATCH] D45006: [Tooling] A CompilationDatabase wrapper that infers header commands.

ilya-biryukov added inline comments. Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:96 +// Sort input for determinism (index is used as a tiebreaker). +llvm::sort(OriginalPaths.begin(), OriginalPaths.end()); +for (size_t I = 0; I < Filenames.size(); ++I)

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

ilya-biryukov added a comment. Thanks, this patch raises a very good point. Having a VFS that is not overlayed over RealFS is almost always the wrong thing to do. On the other hand, I think it's useful to have the client code mention that it overlays over real filesystem, rather than relying on

[PATCH] D45285: [clangd-vscode] Update VScode dependencies

ilya-biryukov accepted this revision. ilya-biryukov added a reviewer: ilya-biryukov. ilya-biryukov added a comment. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45285 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:412 +llvm::raw_string_ostream OS(Message); +OS << "method not found (" << Method << ")"; +replyError(ErrorCode::MethodNotFound, OS.str()); simark wrote: > ilya-biryukov wrote:

[PATCH] D45409: [cmake] Include LLVMTestingSupport when doing stand-alone build

ilya-biryukov added a subscriber: sammccall. ilya-biryukov added a comment. This LG to fix the standalone build. However, why not install LLVMTestingSupport in llvm? It feels that we should either include it or disable the tests for standalone builds. And disabling the tests seems wrong. WDYT? @

[PATCH] D45285: [clangd-vscode] Update VScode dependencies

ilya-biryukov added a subscriber: hokein. ilya-biryukov added a comment. In https://reviews.llvm.org/D45285#1058567, @malaperle wrote: > Do we need to bump the version of the extension and do a new release or > anything like that? Or leave this for later? We should bump the version and republi

[PATCH] D45356: [clangd] Adapt index interfaces to D45014, and fix the old bugs.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45356 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45006: [Tooling] A CompilationDatabase wrapper that infers header commands.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a small nit. Really excited about this landing! Comment at: lib/Tooling/InterpolatingCompilationDatabase.cpp:353 + } + Best = Candidate.firs

[PATCH] D45258: [clang-tidy] Return non-zero exit code for clang errors.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D45258 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

ilya-biryukov added inline comments. Comment at: include/clang/Basic/VirtualFileSystem.h:313 +/// \brief Creates a \p vfs::OverlayFileSystem which overlays the given file +/// system above the 'real' file system, as seen by the operating system. +IntrusiveRefCntPtr --

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

ilya-biryukov added inline comments. Comment at: include/clang/Basic/VirtualFileSystem.h:315 +IntrusiveRefCntPtr +createOverlayOnRealFilesystem(IntrusiveRefCntPtr TopFS); + NIT: I'm not an expert in English, but shouldn't it be createOverlay**Over**Real. Als

[PATCH] D45095: [clang-tidy] Align usage of ClangTool interface with new VFS injection

ilya-biryukov added subscribers: hokein, alexfh. ilya-biryukov added inline comments. Comment at: clang-tidy/ClangTidy.cpp:91 public: - ErrorReporter(ClangTidyContext &Context, bool ApplyFixes, -llvm::IntrusiveRefCntPtr BaseFS) - : Files(FileSystemOptions()

[PATCH] D45014: [Index] Return SourceLocation to consumers, not FileID/Offset pair.

ilya-biryukov added a comment. LGTM, but let's watch out for @akyrtzi's and @arphaman's comments, just in case they're not happy with the change. In that case we'll have to revert it. Repository: rC Clang https://reviews.llvm.org/D45014 ___ cfe-

[PATCH] D45478: [clangd] Merge symbols in global-sym-builder on the go

ilya-biryukov created this revision. ilya-biryukov added reviewers: hokein, sammccall, klimek. Herald added subscribers: MaskRay, ioeric, jkorous-apple. This avoids storing intermediate symbols in memory, most of which are duplicates. The resulting .yaml file is ~120MB, while intermediate symbols

[PATCH] D45482: [clangd] Match AST and Index label for template Symbols

ilya-biryukov created this revision. ilya-biryukov added reviewers: sammccall, hokein. Herald added subscribers: MaskRay, ioeric, jkorous-apple, klimek. Previsouly, class completions items from the index were missing template parameters in both the snippet and the label. Repository: rCTE Clang

[PATCH] D45442: Parse .h files as objective-c++ if we don't have a compile command.

ilya-biryukov added a comment. ObjC++ definitely seems like a nicer default. Unfortunately, we'll start getting ObjC++ completion results, which may be confusing. But that seems like a smaller evil. Is there an easy way to add a regression test that checks we don't get any errors for headers w

[PATCH] D45479: [Tooling] Optimize memory usage in InMemoryToolResults.

ilya-biryukov added a comment. This definitely helps with memory consumption of clangd's global-symbol-indexer, I can now run it over LLVM on MacBook without it swapping out of RAM. The physical memory usage goes up to 3GB at some point, before it was > 20GB. Strictly speaking, interning the s

[PATCH] D45442: Parse .h files as objective-c++ if we don't have a compile command.

ilya-biryukov added inline comments. Comment at: clangd/GlobalCompilationDatabase.cpp:24 + // Parsing as Objective C++ is friendly to more cases. + if (llvm::sys::path::extension(File) == ".h") +Argv.push_back("-xobjective-c++-header"); sammccall wrote: > i

[PATCH] D35894: [clangd] Code hover for Clangd

ilya-biryukov added a comment. Sorry for late response, was on vacation. +1 to all @malaperle 's comments. Here are some extra quick comments, will take a closer look later. Comment at: clangd/ClangdServer.cpp:11 #include "ClangdServer.h" +#include "Protocol.h" #include "cla

[PATCH] D39676: [clangd] Add rename support.

ilya-biryukov added a comment. Just a few minor code style comments. Comment at: clangd/ClangdServer.cpp:367 +Context.setASTContext(AST->getASTContext()); +auto rename = clang::tooling::RenameOccurrences::initiate( +Context, SourceRange(SourceLocationBeg), NewNa

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. I don't think we should pass the very general configuration map to `ClangdServer`. Especially given that we can easily update `DirectoryBaseCompilationDatabase` instance in `ClangdLSPServer` itself. What are your th

[PATCH] D39375: [clang] Add PPCallbacks list to preprocessor when building a preacompiled preamble.

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Could you please take a different approach and add the callbacks you need into `PreambleCallbacks` class? It already has this one: virtual void HandleMacroDefined(con

[PATCH] D39797: [clangd] Relax definitions.test to accept windows file paths.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D39797 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D39799: [Tooling] Use FixedCompilationDatabase when `compile_flags.txt` is found.

ilya-biryukov added a comment. I can see that `FixedCompilationDatabase` does not set a working directory. Is this something we may want to have for `compile_flags.txt` or one would need to resort to `compile_commands.json` to get this? E.g., I'd find it useful to add includes with paths relativ

[PATCH] D39799: [Tooling] Use FixedCompilationDatabase when `compile_flags.txt` is found.

ilya-biryukov added a comment. In https://reviews.llvm.org/D39799#919415, @sammccall wrote: > Currently the working directory is always the parent of `compile_flags.txt` > as you suggest. > Maybe this isn't great though - sometimes it might be important that the WD > is the parent of the sourc

[PATCH] D39799: [Tooling] Use FixedCompilationDatabase when `compile_flags.txt` is found.

ilya-biryukov added a comment. In https://reviews.llvm.org/D39799#919494, @sammccall wrote: > e.g. IIUC, things like `#include "sibling.h"` won't look for the h file next > to the cc file? Actually, this should work as quoted includes are always searched relative to the current file before lo

<    25   26   27   28   29   30   31   32   33   >