[PATCH] D55648: [CodeComplete] Fill preferred type on binary expressions

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Repository: rC Clang https://reviews.llvm.org/D55648 Files: include/clang/Sema/Sema.h lib/Parse/ParseExpr.cpp lib/Sema/SemaCodeComplete.cpp unittests/Sema/CodeCompleteTest.cpp Index: unittests/Sema/CodeComp

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

ilya-biryukov added inline comments. Comment at: clangd/index/Background.cpp:107 while (ThreadPoolSize--) { ThreadPool.emplace_back([this] { run(); }); } NIT: remove braces around a single statement Comment at: clangd/index/Backgroun

[PATCH] D55417: [clangd] Change disk-backed storage to be atomic

ilya-biryukov added inline comments. Comment at: clangd/index/BackgroundIndexStorage.cpp:42 + llvm::Twine TempPath(OutPath, ".tmp."); + TempPath.concat(std::to_string(rand())); + std::error_code EC; There's a helper in LLVM that will do this, `llvm::createUniq

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

ilya-biryukov added a comment. 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 "building preamble", "building file" or "queued" in the status bar can be useful to the us

[PATCH] D55437: [Index] Index declarations in lambda expression.

ilya-biryukov added a comment. The `RecursiveASTVisitor` seems to have the code that properly traverses parameters and return types of lambdas. Any ideas why it's not getting called here? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55437/new/ https://reviews.

[PATCH] D55649: [clangd] Enable cross-namespace completions by default in clangd

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM. When we know everyone is happy with the new behaivour, we can remove the flag altogether. Comment at: clangd/tool/ClangdMain.cpp:144 "can insert scope qualifiers."), -cl::init(false), cl::H

[PATCH] D55648: [CodeComplete] Fill preferred type on binary expressions

ilya-biryukov updated this revision to Diff 178069. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Address review comments Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55648/new/ https://reviews.llvm.org/D55648 Files: include

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

ilya-biryukov added inline comments. Comment at: clangd/index/Background.h:121 bool ShouldStop = false; - std::deque Queue; + std::list> Tasks; std::vector ThreadPool; // FIXME: Abstract this away. Why not `std::deque`? Repository: rCTE Clang Tools Ex

[PATCH] D55648: [CodeComplete] Fill preferred type on binary expressions

ilya-biryukov added a comment. Thanks for the review! Comment at: lib/Sema/SemaCodeComplete.cpp:4928 +if (LHSType->isIntegralOrEnumerationType()) + return S.getASTContext().IntTy; +return QualType(); kadircet wrote: > why not LHSType ? The shifts a

[PATCH] D55648: [CodeComplete] Fill preferred type on binary expressions

ilya-biryukov updated this revision to Diff 178071. ilya-biryukov added a comment. Herald added a subscriber: arphaman. - Update the test after changes Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55648/new/ https://reviews.llvm.org/D55648 Files: include/clan

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

ilya-biryukov added inline comments. Comment at: clangd/index/Background.h:121 bool ShouldStop = false; - std::deque Queue; + std::list> Tasks; std::vector ThreadPool; // FIXME: Abstract this away. kadircet wrote: > ilya-biryukov wrote: > > Why not `std::

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

ilya-biryukov added a comment. Good point, we should definitely state it's a clangd-specific extension. We could come up with a naming scheme for our extensions. I would propose something like `clangd:$methodName`, i.e. the current extensions would be `clangd:textDocument/fileStatus`. WDYT? R

[PATCH] D55315: [clangd] Only reduce priority of a thread for indexing.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clangd/index/Background.h:121 bool ShouldStop = false; - std::deque Queue; + std::deque> Tasks; std::vector ThreadPool; // FIXME: Abstr

[PATCH] D55417: [clangd] Change disk-backed storage to be atomic

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a few nits Comment at: clangd/index/BackgroundIndexStorage.cpp:39 +template +llvm::Error writeAtomically(llvm::StringRef OutPath, WriterFunction &&Wr

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added a comment. Probably the most important suggestion from my side is trying to clearly separate the `enqueue` calls (which actually schedule rebuilds of TUs) using clang and the `loadShards` function. The index loading should be fast, so I assume we won't win much latency by sc

[PATCH] D55437: [Index] Index declarations in lambda expression.

ilya-biryukov added a comment. Update after investigating with @hokein offline: the `RecursiveASTVisitor` has custom code to visit lambda parameters and return type, but it fallback to visiting typelocs when both parameters and a return type are specified. Unfortunately this fallback does not w

[PATCH] D55815: DO NOT SUBMIT. [clangd] A helper Mutex class that reports contention stats

ilya-biryukov created this revision. Herald added subscribers: cfe-commits, kadircet, jfb, arphaman, jkorous, MaskRay, ioeric. Useful in getting some stats of a contention on a particular mutex. Currently it only reports the sum of wall time taken by all threads waiting on a mutex. Repository:

[PATCH] D54945: This commit adds a chapter about clang-tidy integrations

ilya-biryukov added a comment. Thanks for the update! We still need to solve the technicality here: could you please upload the diff with full context? See LLVM docs for an example on how this can be done with var

[PATCH] D55818: [clangd] Unify path canonicalizations in the codebase

ilya-biryukov added a comment. The change mostly, the only important comment from me is about `toURI`. I believe it would actually have an observable effect in the real world and I'm not sure things won't break after it. Could we avoid changing it and focus only on tweaking the "canonical" path

[PATCH] D55820: [AST] Unify the code paths of traversing lambda expressions.

ilya-biryukov added inline comments. Comment at: include/clang/AST/RecursiveASTVisitor.h:2416 FunctionProtoTypeLoc Proto = TL.getAsAdjusted(); + if (S->hasExplicitParameters()) { Could we add a comment on why we can't simply call the `TraverseTypeLoc` here?

[PATCH] D55437: [Index] Index declarations in lambda expression.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: lib/Index/IndexBody.cpp:464 + if (DC && isLambdaCallOperator(DC)) { +IndexCtx.handleDecl(D); + } NIT: remove

[PATCH] D55820: [AST] Unify the code paths of traversing lambda expressions.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. The new code looks simpler and it's arguably simpler for the clients, since they'll have a consistent set of callbacks independent of the actual view into the lambda expres

[PATCH] D55818: [clangd] Unify path canonicalizations in the codebase

ilya-biryukov accepted this revision. ilya-biryukov added inline comments. This revision is now accepted and ready to land. Comment at: clangd/SourceCode.cpp:203 +FilePath)) { + elog("Could not turn relative path to absolute: {0}: {1}", FilePath, +

[PATCH] D55885: [CodeComplete] Properly determine qualifiers of 'this' in a lambda

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. The clang used to pick up the qualifiers of the lamba's call operator (which is always const) and fail to show non-const methods of 'this' in completion results. Repository: rC Clang https://reviews.llvm.org/D55885

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

ilya-biryukov added a comment. Could we add a capability to the init request to check for this extension? We don't want to send those notifications to clients that don't have the code to handle them. Another observation: when I played around with the previous version of the patch, `RunningActi

[PATCH] D55918: [clangd] Don't miss the expected type in merge.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55918/new/ https://reviews.llvm.org/D55918 _

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

ilya-biryukov added inline comments. Comment at: clangd/Protocol.cpp:769 O.map("fallbackFlags", Opts.fallbackFlags); + O.map("fileStatus", Opts.FileStatus); return true; Ah, there's a non-zero chance of name clash here in case the protocol implements some

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

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 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55363/new/ https://reviews.llvm.org/D55363 __

[PATCH] D55374: [clangd] Show FileStatus in vscode-clangd.

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 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55374/new/ https://reviews.llvm.org/D55374 __

[PATCH] D55994: [clangd] Check preceding char when completion triggers on ':' or '>'

ilya-biryukov created this revision. ilya-biryukov added a reviewer: hokein. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric. Only run completion when we were trigerred on '->' and '::', otherwise send an error code in return. To avoid automatically invoking completions in c

[PATCH] D55994: [clangd] Check preceding char when completion triggers on ':' or '>'

ilya-biryukov added a comment. This still needs tests, will add them before committing the change. However, the implementation should be good for review. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55994/new/ https://reviews.llvm.org/D55994 __

[PATCH] D55994: [clangd] Check preceding char when completion triggers on ':' or '>'

ilya-biryukov updated this revision to Diff 179844. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Address comments - Added a test Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55994/new/ https://reviews.llvm.org/D5

[PATCH] D55994: [clangd] Check preceding char when completion triggers on ':' or '>'

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:811 + if (Trigger == ">") +return (*Code)[*Offset - 2] != '-'; // trigger only on '->'. + if (Trigger == ":") hokein wrote: > Checking `Offset` is not always right for rare cases li

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

ilya-biryukov added a comment. In D55363#1343611 , @jkorous wrote: > I am a bit late to the discussion but isn't this something to be dealt with > on the client's side? That would mean more complex code in **every** client and more communication betwee

[PATCH] D55994: [clangd] Check preceding char when completion triggers on ':' or '>'

ilya-biryukov added a comment. In D55994#1343669 , @jkorous wrote: > This looks like a work around LSP imperfection indeed. Totally. > Are you going to push for change in LSP? Something like > `CompletionOptions/triggerCharacters` -> `CompletionOptions

[PATCH] D56267: [clangd] Interfaces for writing code actions

ilya-biryukov created this revision. ilya-biryukov added reviewers: kadircet, ioeric, hokein, sammccall. Herald added subscribers: arphaman, jkorous, MaskRay, mgorny. The code actions can be run in two stages: - Stage 1. Decides whether the action is available to the user and collects all the in

[PATCH] D56267: [clangd] Interfaces for writing code actions

ilya-biryukov added a comment. That's the result of me playing around with writing a small code action for clangd. It's not final yet, e.g. missing tests and example actions, but still wanted to drop the change here to make sure it's not lost, I believe we'll need something like this in the nea

[PATCH] D55994: [clangd] Check preceding char when completion triggers on ':' or '>'

ilya-biryukov added a comment. In D55994#1344717 , @jkorous wrote: > @ilya-biryukov Yes, that's true. I would still expect some performance gain > in more restrictive filtering on client's side - not sure how big though. > Anyway, seems like LSP folks th

[PATCH] D56263: [clangd] Always try to build absolute path

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM NIT: a typo in the change description: s/smylinks/symlinks Maybe also add a separate sentence to the description that this should not affect the behavior if file is not a sy

[PATCH] D56061: [clang-tools-extra] [clangd] Fix detecting atomics in stand-alone builds

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks for the fix. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56061/new/ https://reviews.llvm.org/D56061 _

[PATCH] D56380: [clangd] Fix a regression issue caused by r348365.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56380/new/ https://reviews.llvm.org/D56380 __

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

ilya-biryukov added a comment. @hokein, do you need reviewers for this? I'm happy to volunteer. In D56314#1347511 , @nridge wrote: > Might we want to keep some of this information for `workspace/symbol`? I > mean, surely not "documentation", but perhaps

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

ilya-biryukov added inline comments. Comment at: clangd/index/Index.h:232 /// See also isIndexedForCodeCompletion(). +/// Note that we don't store completion information (signature, snippet, +/// documentation, type, inclues, etc) if the symbol is not indexed for co

[PATCH] D56442: [clangd] Fix a crash when reading an empty index file.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks for fixing this. A few questions for the long-term: - Should we consider removing the YAML support altogether? - If we want to keep, are there non-private APIs that

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

ilya-biryukov added inline comments. Comment at: clangd/index/SymbolCollector.cpp:543 + + if (!(S.Flags & Symbol::IndexedForCodeCompletion)) +return Insert(S); hokein wrote: > ilya-biryukov wrote: > > Most of the fields updated at the bottom aren't useful. H

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added a comment. Most comments are NITs, the major one that I'd suggest paying most attention to is about rewriting newer results with an older ones. Comment at: clangd/index/Background.cpp:259 + // if this thread sees the older version but finishes later. T

[PATCH] D56446: [Driver] Fix libcxx detection on Darwin with clang run as ./clang

ilya-biryukov created this revision. ilya-biryukov added reviewers: arphaman, thakis. Herald added a reviewer: EricWF. Herald added a subscriber: christof. By using '..' instead of fs::parent_path. The intention of the code was to go from 'path/to/clang/bin' to 'path/to/clang/include'. In most ca

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added inline comments. Comment at: clangd/index/Background.cpp:259 + // if this thread sees the older version but finishes later. This should + // be rare in practice. + DigestIt.first->second = Hash; kadircet wrote: > ilya-biryukov w

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added inline comments. Comment at: clangd/SourceCode.h:107 +// "/tmp/build/foo.h" +std::string makeCanonicalPath(llvm::StringRef AbsolutePath, + const SourceManager &SM); This changes should go away after the rebase, rig

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added inline comments. Comment at: clangd/index/Background.cpp:197 +Cmd.CommandLine.push_back("-resource-dir=" + ResourceDir); +const std::string FileName = Cmd.Filename; +if (auto Error = index(std::move(Cmd), Storage)) use

[PATCH] D56483: [clangd] Add a test for SignatureHelp on dynamic index.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Many thanks! Should've been there from the start, sorry about the inconvenience Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56483/

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clangd/index/Index.h:188 /// candidate list. For example, "(X x, Y y) const" is a function signature. + /// This field is only meaningful w

[PATCH] D56446: [Driver] Fix libcxx detection on Darwin with clang run as ./clang

ilya-biryukov added a comment. Sorry about the delay. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56446/new/ https://reviews.llvm.org/D56446 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added inline comments. Comment at: clangd/SourceCode.h:21 #include "clang/Tooling/Core/Replacement.h" +#include "llvm/Support/Path.h" #include "llvm/Support/SHA1.h" Looks redundant. Remove? Comment at: clangd/index/Background.cp

[PATCH] D56314: [clangd] Don't store completion info if the symbol is not used for code completion.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM again :-) I bet the savings are less now that we're always storing the comments in the static index, so the numbers in the description might be outdated. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D56492: [clangd] Add Documentations for member completions.

ilya-biryukov added inline comments. Comment at: clangd/CodeComplete.cpp:1373 // Convert the results to final form, assembling the expensive strings. -for (auto &C : Top) { - Output.Completions.push_back(toCodeCompletion(C.first)); - Output.Completions.back().S

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added inline comments. Comment at: clangd/URI.h:131 +/// Resolves URI to file paths with cache. +class URIToFileCache { +public: Maybe move it into the `Backgroud.cpp`, e.g. as a private member of `BackoundIndex`? We don't have other use-case for i

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added a comment. As discussed offline, let's remove the parts of the change that were aiming to fix the consistency issues, the issues were there before this patch, the fix is getting complicated and doesn't really solve all of the problems. All of that suggests it's out of scope

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov added a comment. Only one important comment about bringing back the comment. Comment at: clangd/index/Background.cpp:224 +auto NeedsReIndexing = loadShards(std::move(ChangedFiles)); +// Run indexing for files that needs to be updated. +std:

[PATCH] D55224: [clangd] Introduce loading of shards within auto-index

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Ship it! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55224/new/ https://reviews.llvm.org/D55224

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov updated this revision to Diff 219503. ilya-biryukov marked 6 inline comments as done. ilya-biryukov added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67341/new/ https://reviews.llvm.org/D67341 Files:

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:169 private: - void addTokenForTypedef(SourceLocation Loc, const TypedefNameDecl *TD) { -auto *TSI = TD->getTypeSourceInfo(); -if (!TSI) - return; -// Try to high

[PATCH] D67584: [Support] Replace function with function_ref in writeFileAtomically. NFC

ilya-biryukov created this revision. ilya-biryukov added a reviewer: jkorous. Herald added subscribers: llvm-commits, jfb, dexonsmith, hiraditya. Herald added a project: LLVM. The latter is slightly more efficient and communicates the intent of the API: writeFileAtomically does not own or copy the

[PATCH] D67584: [Support] Replace function with function_ref in writeFileAtomically. NFC

ilya-biryukov updated this revision to Diff 220209. ilya-biryukov added a comment. - Reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67584/new/ https://reviews.llvm.org/D67584 Files: llvm/include/llvm/Support/FileUtilities.h llvm/lib/Su

[PATCH] D67341: [clangd] Simplify semantic highlighting visitor

ilya-biryukov added a comment. @hokein, do you want to do another round or is this good to go? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67341/new/ https://reviews.llvm.org/D67341 ___ cfe-commits m

[PATCH] D67607: [clangd] Fix a crash when renaming operator.

ilya-biryukov added a comment. LGTM to fix the crash. See the comment about more cases that could potentially break, though. Do we handle them gracefully now? Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:77 return ReasonToReject::UnsupportedSymbol; + if (cons

[PATCH] D67654: [clang-tidy] Fix a potential infinite loop in readability-isolate-declaration check.

ilya-biryukov added a comment. Do we have unit tests for clang-tidy? If yes, could we also add unit-tests for this function? The modified function only needs a source manager and lang options, these are easy to mock. Comment at: clang-tools-extra/clang-tidy/utils/LexerUtils.

[PATCH] D67654: [clang-tidy] Fix a potential infinite loop in readability-isolate-declaration check.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. The fix itself LGTM, getting rid of an infinite loop is important. All my comment can be addressed later... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D67584: [Support] Replace function with function_ref in writeFileAtomically. NFC

ilya-biryukov reopened this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Ah, the tests fail to compile because we now have an overload where the only difference is `function_ref` vs `StringRef`. This works fine with `std::function`, STL probably ensure

[PATCH] D67654: [clang-tidy] Fix a potential infinite loop in readability-isolate-declaration check.

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clang-tidy/utils/LexerUtils.h:38 template SourceLocation findPreviousAnyTokenKind(SourceLocation Start, const SourceManag

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

ilya-biryukov added a comment. A drive-by comment... Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1135 + Params.positions.size()); +Reply(llvm::make_error("failed to decode request", + ErrorCode::InvalidRequest));

[PATCH] D67720: [clangd] Add semantic selection to ClangdLSPServer.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1131 +Callback> Reply) { + if (Params.positions.size() != 1) { +elog("{0} positions provided to SelectionRange. Supports exactly one " hokein wrote: > maybe add

[PATCH] D66647: [clangd] DefineInline action apply logic with fully qualified names

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp:158 + // i.e: a::Bar> instead of a::Bar> + printTemplateArgumentList(OS, TSTL.getTypePtr()->template_arguments(), +TD->getASTConte

[PATCH] D67825: [AST] Extrat Decl::printQualifier helper from Decl::printQualifiedName.

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added a subscriber: usaxena95. Herald added a project: clang. To be used in clangd, e.g. in D66647 . Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D67825 File

[PATCH] D67826: [clangd] A helper to find explicit references and their names

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added subscribers: usaxena95, arphaman, jkorous, MaskRay. Herald added a project: clang. Allows to simplify pending code tweaks: - the upcoming DefineInline tweak (D66647 ) - rem

[PATCH] D67827: [clangd] Simplify name qualification in DefineInline

ilya-biryukov added a comment. It's probably better to land this as part of D66647 , posting this here for demonstrative purposes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67827/new/ https://reviews.llvm.org/D6

[PATCH] D67827: [clangd] Simplify name qualification in DefineInline

ilya-biryukov created this revision. Herald added subscribers: usaxena95, arphaman, jkorous, MaskRay. Herald added a project: clang. ilya-biryukov added parent revisions: D67826: [clangd] A helper to find explicit references and their names, D67825: [AST] Extrat Decl::printQualifier helper from D

[PATCH] D67825: [AST] Extract Decl::printNestedNameSpecifier helper from Decl::printQualifiedName

ilya-biryukov updated this revision to Diff 221002. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Update comments - Rename new function to printNestedNameSpecifier Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6782

[PATCH] D67825: [AST] Extract Decl::printNestedNameSpecifier helper from Decl::printQualifiedName

ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/Decl.h:313 + /// Returns only qualifier from printQualifiedName, including the :: at the + /// end. E.g. aaron.ballman wrote: > This doesn't return anything, so I think a better way to p

[PATCH] D67825: [AST] Extract Decl::printNestedNameSpecifier helper from Decl::printQualifiedName

ilya-biryukov added a comment. In D67825#1676528 , @kadircet wrote: > LGTM, I know it is trivial, but some more unittests wouldn't hurt anyone :D Yeah, good point, I'll add a few besides tests for `printQualifiedName`. > Also could you update the summar

[PATCH] D67825: [AST] Extract Decl::printNestedNameSpecifier helper from Decl::printQualifiedName

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/Decl.h:317-318 + ///this function returns "A::B::". + void printQualifier(raw_ostream &OS) const; + void printQualifier(raw_ostream &OS, const PrintingP

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

ilya-biryukov added a comment. Sorry for the delay Comment at: clang-tools-extra/clangd/Macro.h:59 + +if (auto Range = getTokenRange(SM, LangOpts, MacroNameTok.getLocation())) { + MainFileMacros.push_back( Converting here is too early, could we keep th

[PATCH] D67826: [clangd] A helper to find explicit references and their names

ilya-biryukov updated this revision to Diff 221345. ilya-biryukov marked 18 inline comments as done. ilya-biryukov added a comment. Herald added subscribers: jfb, mgrang. - Add simple tests - Address other comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D67826: [clangd] A helper to find explicit references and their names

ilya-biryukov added a comment. Added some tests too. More tests for dependent constructs are still missing, but please take a look at what we have for now. Comment at: clang-tools-extra/clangd/FindTarget.cpp:373 + DeclRelation::TemplateInstantiation)) && +

[PATCH] D67501: [clang-tidy] Fix relative path in header-filter.

ilya-biryukov added a comment. In D67501#1679050 , @xyb wrote: > `-normalized-header-filter`. I'd like this idea. Any objections? Having two solution for filter is unfortunate - making both discoverable and documenting why we need them is hard. If there

[PATCH] D38446: update comments in clang-format.py for python3 compatibility

ilya-biryukov added a comment. @MyDeveloperDay, added you on the assumption that you are interested in all things about `clang-format`. Hope the change is relevant, let me know if not. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D38446/new/ https://reviews.llvm

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Macro.h:59 + +if (auto Range = getTokenRange(SM, LangOpts, MacroNameTok.getLocation())) { + MainFileMacros.push_back( hokein wrote:

[PATCH] D67496: [clangd] Collect macros in the preamble region of the main file

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks. A ton of small NITs too, but they're all small details. Comment at: clang-tools-extra/clangd/Macro.h:1 +//===--- Macro.h

[PATCH] D67826: [clangd] A helper to find explicit references and their names

ilya-biryukov marked 9 inline comments as done. ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.cpp:432 + Ref = ReferenceLoc{ + E->getQualifierLoc(), E->getNameInfo().getLoc(), {E->getDecl()}}; +} kadircet wr

[PATCH] D67826: [clangd] A helper to find explicit references and their names

ilya-biryukov updated this revision to Diff 221522. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Use DeclRefExpr::getFoundDecl, add a test - Update a comment - Remove redundant anon namespace declaration - Remove :wa - Do not filter-out macro references - Docume

[PATCH] D67826: [clangd] A helper to find explicit references and their names

ilya-biryukov updated this revision to Diff 221556. ilya-biryukov marked 8 inline comments as done. ilya-biryukov added a comment. - Remove ExprMemberBase - Add an overload for Decl* Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67826/new/ https://

[PATCH] D67826: [clangd] A helper to find explicit references and their names

ilya-biryukov added a comment. I believe all comments were addressed. PTAL and let me know if I missed something. Comment at: clang-tools-extra/clangd/FindTarget.h:93 + /// in 'obj.foo'. + const Expr* MemberExprBase = nullptr; +}; kadircet wrote: > do we hav

[PATCH] D67901: [clangd] Improve semantic highlighting in dependent contexts (fixes #154)

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:181 + addToken(E->getMemberLoc(), E->getQualifier() + ? HighlightingKind::StaticField + : Highlighting

[PATCH] D67973: [libTooling][NFC] Switch StencilTest.cpp to use EXPECT_THAT_EXPECTED

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/D67973/new/ https://reviews.llvm.org/D67973 _

[PATCH] D67826: [clangd] A helper to find explicit references and their names

ilya-biryukov updated this revision to Diff 221728. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - s/Decl* S/Decl* D/g Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67826/new/ https://reviews.llvm.org/D67826 Files

[PATCH] D68027: [clangd] Change constness of parameters to findExplicitRefs

ilya-biryukov added a comment. Who are the callers? Selection tree? FWIW, I'd rather pass non-const everywhere, the concept of const-ness is just not very useful for AST nodes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68027/new/ https://revie

[PATCH] D68027: [clangd] Change constness of parameters to findExplicitRefs

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. @kadircet says we're using this pattern all over clangd. This somehow slipped my view before. LGTM to be consistent with the rest of clangd (and functions in `FindTarget.h`). I t

[PATCH] D68024: [clangd] Implement GetEligiblePoints

ilya-biryukov added a comment. A few go-by comments from me, Haojian should have better context here! Comment at: clang-tools-extra/clangd/SourceCode.cpp:655 +lex(llvm::StringRef Code, const format::FormatStyle &Style, +llvm::function_ref A) { // FIXME: InMemoryFileAdapt

[PATCH] D68077: [clangd][vscode] Turn on the semantic highlighting by default.

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/D68077/new/ https://reviews.llvm.org/D68077 _

[PATCH] D67964: [clangd] Update vscode lsp dependencies to pickup the new changes in LSP v3.15.0.

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/D67964/new/ https://reviews.llvm.org/D67964 _

<    15   16   17   18   19   20   21   22   23   24   >