[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

ilya-biryukov updated this revision to Diff 126345. ilya-biryukov added a comment. - Use `derive() &&` instead of `derive() const &&`. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40485 Files: clangd/CMakeLists.txt clangd/Context.cpp clangd/Context.h clangd/TypedValue

[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

ilya-biryukov added inline comments. Comment at: clangd/Context.h:92 + ContextBuilder derive() const &; + ContextBuilder derive() const &&; + sammccall wrote: > `&&`, not `const&&` :-) > > Maybe add a trailing `//takes ownership` Right. Copy-paste is gonna kil

[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

ilya-biryukov updated this revision to Diff 126346. ilya-biryukov added a comment. - Removed buildCtx(). Now Contexts can only be created using emptyCtx().derive() Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40485 Files: clangd/CMakeLists.txt clangd/Context.cpp clangd/

[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

ilya-biryukov updated this revision to Diff 126373. ilya-biryukov added a comment. - Removed ContextBuilder and TypedValueMap. - Updated the docs. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40485 Files: clangd/CMakeLists.txt clangd/Context.cpp clangd/Context.h unitt

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126375. ilya-biryukov added a comment. - Use derive(key, val) instead of derive().add(key, val). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40486 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.

[PATCH] D40488: [clangd] Implemented tracing using Context

ilya-biryukov updated this revision to Diff 126376. ilya-biryukov added a comment. - Use derive(key, value) instead of derive().add(key, value). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40488 Files: clangd/ClangdUnit.cpp clangd/JSONRPCDispatcher.cpp clangd/Trace.cpp

[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

ilya-biryukov marked an inline comment as done. ilya-biryukov added inline comments. Comment at: clangd/Context.h:65 + Context *Parent; + TypedValueMap Data; +}; klimek wrote: > sammccall wrote: > > ilya-biryukov wrote: > > > sammccall wrote: > > > > ilya-biryu

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:36 - ~FulfillPromiseGuard() { Promise.set_value(); } + ~FulfillContextPromiseGuard() { Promise.set_value(std::move(Ctx)); } sammccall wrote: > Yikes, I can see how we got here, but we

[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

ilya-biryukov added inline comments. Comment at: clangd/Context.h:169 + struct ContextData { +// We need to make sure Parent outlives the Value, so the order of members +// is important. We do that to allow classes stored in Context's child sammccall wro

[PATCH] D40485: [clangd] Introduced a Context that stores implicit data

ilya-biryukov updated this revision to Diff 126515. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Change derive() signature to accept a value instead of the variadic arguments. - Rewrite a while loop into a for loop. - Return a value from Context::empty() instead

[PATCH] D38425: [clangd] Document highlights for clangd

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM minor a slight tweak to fix compilation. I'll land this today. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D38425 _

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126544. ilya-biryukov marked 7 inline comments as done. ilya-biryukov added a comment. - Added a helper that runs functions in destructor. - Fixed compilation after rebase. - Got rid of FulfilContextPromiseGuard - Added a FIXME to remove the Ctx typedef.

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov added a comment. I think all the comments are addressed now and this change is ready for another round of review. Let me know if I missed something. Comment at: clangd/ClangdServer.cpp:36 - ~FulfillPromiseGuard() { Promise.set_value(); } + ~FulfillContextProm

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126551. ilya-biryukov added a comment. - Make Context the first parameter everywhere. - Add a SFINAE check to UniqueFunction's constructor to avoid creating it from non-callable objects. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov added a comment. This is now ready for review. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40486 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov added inline comments. Comment at: clangd/Function.h:44 +/// A sfinae-check that Callable can be called with Args... +class = decltype(std::declval()(std::declval()...), + void())> We need this che

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126554. ilya-biryukov added a comment. - Also check that return type of Callable is correct. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40486 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h

[PATCH] D41118: [clangd] Emit ranges for clangd diagnostics, and fix off-by-one positions

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D41118 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cf

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126680. ilya-biryukov added a comment. - Make Context the first parameter in rename and a few helper methods. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40486 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126683. ilya-biryukov added a comment. These should be the last usages where Context is not a first paramter. - Make Context the first parameter in findDefinitions and CppFile::rebuild. - Make Context the first parameter in invokeCodeCompletion. Repos

[PATCH] D40562: [Sema] Ignore decls in namespaces when global decls are not wanted.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. I'll LGTM this to unblock @ioeric . We'll also get a test using the flag in clangd soon. @arphaman, let us know if you feel there's something wrong with the patch. Repository: rC Clang https://reviews.llvm.org/D40562 __

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126712. ilya-biryukov marked 13 inline comments as done. ilya-biryukov added a comment. - Removed obsolete comments. - Moved ScopeExitGuard to namespace detail. - Added FIXMEs to pass context to compilation database. - Addded a FIXME to JSONOutput. - Rem

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov marked 13 inline comments as done. ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:216 -std::future> -ClangdServer::codeComplete(PathRef File, Position Pos, +std::future, Context>> +ClangdServer::codeComplete(Context Ctx, PathRef File, Pos

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126725. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Simplify ScopeExitGuard by using llvm::Optional<>. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40486 Files: clangd/ClangdLSPServer.cpp cla

[PATCH] D40486: [clangd] Implemented logging using Context

ilya-biryukov updated this revision to Diff 126728. ilya-biryukov added a comment. Merged with head Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40486 Files: clangd/ClangdLSPServer.cpp clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/ClangdUnit.cpp clangd/Clangd

[PATCH] D41005: Reuse preamble even if an unsaved file does not exist

ilya-biryukov added inline comments. Comment at: include/clang/Frontend/ASTUnit.h:196 + /// \brief Counter indicating how often the preamble was build in total. + unsigned PreambleCounter; + nik wrote: > Any better name for this one? Otherwise I would suggest r

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. It seems this patch is out of date, we need to merge it with the latests head. Comment at: clangd/DraftStore.cpp:45 std::lock_guard Lock(Mutex); +

[PATCH] D40488: [clangd] Implemented tracing using Context

ilya-biryukov updated this revision to Diff 126748. ilya-biryukov added a comment. Merged with head Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D40488 Files: clangd/ClangdUnit.cpp clangd/JSONRPCDispatcher.cpp clangd/Trace.cpp clangd/Trace.h clangd/tool/ClangdMain.cp

[PATCH] D40489: [clangd] Changed tracing interfaces

ilya-biryukov updated this revision to Diff 126762. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Changed the EventTracer interface to return a callback from beginEvent instead of using begin_event/end_event method pairs. Repository: rCTE Clang Tools Extra

[PATCH] D40489: [clangd] Changed tracing interfaces

ilya-biryukov added inline comments. Comment at: clangd/Trace.h:39 + /// Called when event with \p Name starts. + virtual void begin_event(const ContextData &Ctx, llvm::StringRef Name) = 0; + /// Called when event with \p Name ends. sammccall wrote: > just cal

[PATCH] D41189: [Frontend] Treat function with skipped body as definition

ilya-biryukov created this revision. ilya-biryukov added reviewers: bkramer, sepavloff. This fixes an invalid warning about missing definition of a function when parsing with SkipFunctionBodies=true Repository: rC Clang https://reviews.llvm.org/D41189 Files: include/clang/AST/Decl.h test

[PATCH] D41189: [Frontend] Treat function with skipped body as definition

ilya-biryukov updated this revision to Diff 126787. ilya-biryukov added a comment. - Reverted accidental whitespace change. Repository: rC Clang https://reviews.llvm.org/D41189 Files: include/clang/AST/Decl.h test/Index/skipped_function_bodies.cpp Index: test/Index/skipped_function_bod

[PATCH] D129488: [Sema] Fix evaluation context of immediate functions in default arguments

ilya-biryukov updated this revision to Diff 456663. ilya-biryukov added a comment. - Special-case the std::source_location::current calls, delay until codegen Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129488/new/ https://reviews.llvm.org/D12948

[PATCH] D132941: [DO NOT SUBMIT] [Sema] Delay evaluation of std::source_location::current() in default arguments

ilya-biryukov created this revision. Herald added a subscriber: martong. Herald added a reviewer: shafik. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. They must be evaluated in the context where

[PATCH] D129488: [Sema] Fix evaluation context of immediate functions in default arguments

ilya-biryukov added a reviewer: aaron.ballman. ilya-biryukov added a subscriber: aaron.ballman. ilya-biryukov added a comment. This seems to mostly work and ready for another round of review, but I still need to update the codegen test, it seems it has caught an error with default-initilization

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

ilya-biryukov added a comment. I was also under impression that this should fall out of the C++ semantics, but after playing around with it I am not so sure anymore. > Isn't it wrong to eagerly evaluate _other_ calls in default args, as well? > ISTM that source_location is simply _exposing_ the

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

ilya-biryukov added a comment. @aaron.ballman, that's my reading of the standard as well. Do you think we should proceed with the current approach or is there another direction worth pursuing to make source_location work? In D129488#3760178 , @ChuanqiXu

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

ilya-biryukov created this revision. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Add a warning `-Waccess-vector-incomplete-member`. Inspired by recent changes to libc++ that make some uses act

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

ilya-biryukov added a comment. This warning might be a little verbose and pedantic for Clang, but I still wanted to share the code. I would be more comfortable doing this a `clang-tidy` check, but I don't think there is an easy way to write it outside `Sema` and `Sema` is not readily available

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

ilya-biryukov added a comment. In D133029#3761400 , @gribozavr2 wrote: > In D133029#3761344 , @ilya-biryukov > wrote: > >> [...] I don't think there is an easy way to write it outside `Sema` and >> `Sema` is no

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

ilya-biryukov added a comment. In D129488#3761398 , @jyknight wrote: > I believe this should print `fn=14 fn2=14`. I don't see how we can get that > by special-casing calls to `std::source_location::current`, and indeed, with > this version of the patch

[PATCH] D132945: [clang] Skip re-building lambda expressions in parameters to consteval fns.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Thanks, that's a nice find! Could you also add a test that mentions uses `consteval` functions in lambda captures inside immediate invocations? Something like `consteval_foo([x =

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

ilya-biryukov added a comment. In D133029#3763120 , @shafik wrote: > Do you have an idea of how common this might be? I don't have the numbers yet, but this broke the builds in C++20 mode inside the core libraries we are using after libc++ change

[PATCH] D129488: [Sema] Delay evaluation of std::source_location::current() in default arguments

ilya-biryukov added a comment. In D129488#3762490 , @jyknight wrote: > The more I look at this, the more I feel like, if all the above behaviors are > correct, source_location::current() simply shouldn't be marked consteval at > all. If it was constexpr

[PATCH] D132918: [clang] Fix a crash in constant evaluation

ilya-biryukov added a comment. I was just passing by, but wanted to add more context from our investigation with @kadircet. If variables with incomplete types appear inside non-template `constexpr` function this gets detected by a call to `CheckConstexprFunctionDefinition` inside `ActOnFinishFu

[PATCH] D132797: [clangd] Support renaming virtual methods

ilya-biryukov added a comment. Thanks for the patch! A drive-by comment from me, hopefully useful. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:554 + Req.Subjects = {Base}; + Index.relations(Req, [&](const SymbolID &, const Symbol &Override) { +IDs.insert(Over

[PATCH] D132797: [clangd] Support renaming virtual methods

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:554 + Req.Subjects = {Base}; + Index.relations(Req, [&](const SymbolID &, const Symbol &Override) { +IDs.insert(Override.ID); sammccall wrote: > ilya-biryukov wrot

[PATCH] D132918: [clang] Fix a crash in constant evaluation

ilya-biryukov accepted this revision as: ilya-biryukov. ilya-biryukov added a comment. In D132918#3773237 , @shafik wrote: > This is helpful information but I am not sure this convinces me that > `EvaluateVarDecl` is the correct place to check. Why not i

[PATCH] D132797: [clangd] Support renaming virtual methods

ilya-biryukov added a comment. A few more NITs from me, but please wait for @sammccall for another round of reviews. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:550 +// method. +void recursivelyInsertOverrides(SymbolID Base, llvm::DenseSet &IDs, +

[PATCH] D133029: [Sema] Allow to diagnose the references to std::vector with incomplete T

ilya-biryukov added a subscriber: aaron.ballman. ilya-biryukov added a comment. In D133029#3766058 , @shafik wrote: > This old cfe-dev thread might be relevant: > https://lists.llvm.org/pipermail/cfe-dev/2018-June/058338.html Thanks, the thread is defin

[PATCH] D133415: [clangd] Fix non-idempotent cases of canonicalRenameDecl()

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM to unbreak clangd. Agree that a more thorough look at this is needed. Maybe add a bug to track this? Comment at: clang-tools-extra/clangd/unittests/RenameT

[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

ilya-biryukov added a comment. It seems wrong to change semantics of initialization when instantiating concept requirements. It implies that semantic checking may behave differently inside requires expressions, this is a red flag. Clang has a mechanism

[PATCH] D128411: [syntax] Introduce a BaseToken class.

ilya-biryukov added inline comments. Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:569 struct Forest { -Forest(syntax::Arena &A) { - assert(!A.getTokenBuffer().expandedTokens().empty()); - assert(A.getTokenBuffer().expandedTokens().back().kind() == tok::eof)

[PATCH] D128411: [syntax] Introduce a BaseToken class.

ilya-biryukov added inline comments. Comment at: clang/include/clang/Tooling/Syntax/Tokens.h:463 +/// It tracks the underlying token buffers, source manager, etc. +class SyntaxTokenManager : public TokenManager { +public: sammccall wrote: > conceptually this is j

[PATCH] D128621: [clangd] Do not try to use $0 as a placeholder in completion snippets

ilya-biryukov added a comment. My 2 cents here. We should probably try hard to keep the cursor inside braces/parentheses for those patterns. Having namespace foo { decls }^ <- cursor here hurts UX. It would be nice to get the old behavior back somehow. I still have hopes for the VSCode

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov created this revision. ilya-biryukov added a reviewer: erichkeane. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Currently the C++20 concepts are only merged in `ASTReader`, i.e. when coming from different TU. This can ca

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov added a comment. @erichkeane please let me know if you're the right person to review this. Comment at: clang/lib/Sema/SemaTemplate.cpp:8674 auto *Old = Previous.getRepresentativeDecl(); Diag(NameLoc, isa(Old) ? diag::err_redefinition : diag::er

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov updated this revision to Diff 441466. ilya-biryukov added a comment. - Update code to match how typedefs behave Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128921/new/ https://reviews.llvm.org/D128921 Files: clang/include/clang/S

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov added a comment. I haven't seen your reply before posting my initial comments. Many thanks for a very quick turnaroud on this! Will wait for module folks to approve. Comment at: clang/lib/Sema/SemaTemplate.cpp:8674 auto *Old = Previous.getRepresentativeDecl(

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov updated this revision to Diff 441468. ilya-biryukov added a comment. - remove leftover test from previous version Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128921/new/ https://reviews.llvm.org/D128921 Files: clang/include/clang

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov updated this revision to Diff 442623. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Update code to match how typedefs behave - remove leftover test from previous version - Add test for C++20 modules, rewrite original test with split-file - use isRea

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov added a comment. Many thanks! I didn't know about `split-file`, it's much nicer indeed! Also incorporating feedback from @rsmith and removing the call to `makeMergedDefinitionVisible`. Keeping just `setPrimaryMergedDecl` is enough here. Richard's reply from the email exchange:

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov added a comment. PS I will make sure to look at the patches you sent my way this week. Wanted to do it earlier, but have been having some personal emergencies I needed to take care of. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128

[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

ilya-biryukov updated this revision to Diff 442633. ilya-biryukov added a comment. - Always call PushToScopeChains. This is the right behavior after we stopped calling makeMergedDefinitionVisible Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128921

[PATCH] D130585: [Sema] Return primary merged decl as canonical for concepts

ilya-biryukov created this revision. ilya-biryukov added a reviewer: ChuanqiXu. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Otherwise we get invalid results for ODR checks. See changed test for an example: despite the fact that we me

[PATCH] D130585: [Sema] Return primary merged decl as canonical for concepts

ilya-biryukov added a comment. I wonder whether the right fix for this should generalize to `Mergeable`. The fact that `Mergeable::getFirstDecl` checks if the decl is coming from the AST file before looking up its primary merged decl is probably a performance optimization, not a deliberate seman

[PATCH] D130585: [Sema] Return primary merged decl as canonical for concepts

ilya-biryukov updated this revision to Diff 447978. ilya-biryukov added a comment. - set canonical decl as primary merged, add tests for the relevant failure Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130585/new/ https://reviews.llvm.org/D130585

[PATCH] D130585: [Sema] Return primary merged decl as canonical for concepts

ilya-biryukov added a comment. In D130585#3681397 , @ChuanqiXu wrote: > In D130585#3680189 , @ilya-biryukov > wrote: > >> I wonder whether the right fix for this should generalize to `Mergeable`. >> The fact that

[PATCH] D130585: [Sema] Return primary merged decl as canonical for concepts

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG42f87bb62d07: [Sema] Return primary merged decl as canonical for concepts (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

ilya-biryukov added a comment. Thanks! Mostly questions to better understand the spec and clang. Please excuse me if they sound too basic. I have read the modules section of the standard, but I couldn't find where it specifies whether these redefinition errors should be present or not. Any guid

[PATCH] D130690: [clangd][NFCI] Store TUPath inside ParsedAST

ilya-biryukov added a comment. I want to understand the trade-offs a bit better. - what are the actual differences between the file paths passed to TU and the canonical path in source manager? - what issues does having these differences result in? Having some tests that illustrate the differenc

[PATCH] D130690: [clangd][NFCI] Store TUPath inside ParsedAST

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM and a few nits. Could you please add a summary of our offline discussion here. Comment at: clang-tools-extra/clangd/ParsedAST.cpp:763 -ParsedAST::Parsed

[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

ilya-biryukov added a comment. Thanks for the thorough explanation and testing. LGTM. Comment at: clang/lib/Sema/SemaDecl.cpp:1738 +// Return true if the redefinition is not allowed. Return false otherwise. +bool Sema::CheckRedefinitionInModule(const NamedDecl *New, +

[PATCH] D130864: [NFC] Introduce ASTContext::isInSameModule()

ilya-biryukov added a comment. Thanks for working on this. I have a few major concerns here: - Do we know that it's a bottleneck in practice? E.g. if the module name have different sizes, comparing strings is actually fast. If the module names are short, we are also in a pretty good situation.

[PATCH] D130863: [clangd] Make git ignore index directories

ilya-biryukov added a comment. I'm with Kadir on this one, but maybe I'm too attached to my `.git/info/exclude`s. I somehow don't feel it's a big burden specific to clangd. I have to configure `.gitignore` for my projects and `.git/info/excludes` for other projects I work on because of many too

[PATCH] D130863: [clangd] Make git ignore index directories

ilya-biryukov added a comment. In D130863#3693293 , @sammccall wrote: > In D130863#3693135 , @ilya-biryukov > wrote: > >> Otherwise, I would personally still put `.cache/clangd` into the global >> `.gitignore` o

[PATCH] D131258: [Sema] Merge variable template specializations

ilya-biryukov created this revision. ilya-biryukov added a reviewer: ChuanqiXu. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Clang used to produce redefinition errors, see tests for examples. Repository: rG LLVM Github Monorepo h

[PATCH] D131258: [Sema] Merge variable template specializations

ilya-biryukov updated this revision to Diff 450770. ilya-biryukov added a comment. - Add the forgotten `-fmodules-local-submodule-visibility` flag to the test - Add newlines to the test files - Switch to C++14 mode for tests, they don't use C++17. Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D131258: [Sema] Merge variable template specializations

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. In D131258#3705503 , @ChuanqiXu wrote: > When I run this locally, it emits several unexpected errors: Sorry, my bad. I was testing this with `-std=c++20` and later switched to

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

ilya-biryukov added a comment. Hey! Also wondering what's the status of this. @Rakete do you plan to finish the patch? Or would it be ok if someone takes it over? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53847/new/ https://reviews.llvm.or

[PATCH] D131258: [Sema] Merge variable template specializations

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. ilya-biryukov marked 2 inline comments as done. Closed by commit rG07529996d92b: [Sema] Merge variable template specializations (authored by ilya-biryukov). Changed pr

[PATCH] D131479: Handle explicitly defaulted consteval special members.

ilya-biryukov added a comment. Should we follow suggestion from CWG Issue 2602 to fix this instead? I think the trick is to keep the constructor consteval, but complain when processing its call, e.g. somewhere around `HandleCon

[PATCH] D131479: Handle explicitly defaulted consteval special members.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM from my side. Thanks for explaining, I was confused because I looked at the code right after defaulted check and thought we were working around that particular error. Turns

[PATCH] D131569: [clangd] Allow updates to be canceled after compile flags retrieval

ilya-biryukov added a comment. Could you please add a test with a compilation database that models the blocking behavior, attempts cancellation and ensures no diagnostics are produced? I also suggest adding a callback similar to `Callbacks.onFailedAST`, which could be used for testing this beha

[PATCH] D124250: [Serialization] write expr dependence bits as a single integer

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Nice savings Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:2291 + unsigned ExprDependenceBits = llvm::BitWidth; // Abbreviation for EXPR_DEC

[PATCH] D124420: [Serialization] Compress serialized macro expansion SLocEntries

ilya-biryukov added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:2181 + // Common cases are covered by abbreviations. + unsigned Abbrev = [&]() -> unsigned { +if (!Expansion.isExpansionTokenRange()) // Token splits. NIT: ma

[PATCH] D124422: [Serialization] Improve encoding of small SourceRanges.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Overall LGTM, but I didn't look closely if there are cases that break now. Do we have any tests to check this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D124441: [Index] [clangd] Support for concept declarations and requires expressions

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: usaxena95, kadircet, arphaman. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a subscriber: MaskRay. Herald added projects: clang, clang-tools-extra.

[PATCH] D124422: [Serialization] Improve encoding of small SourceRanges.

ilya-biryukov added a comment. Yeah, that sounds promising. Various source locations inside the same nodes are probably very close. Probably even across nodes. If we are willing to take on a bit more complexity, we could probably get even bigger wins by storing all source locations in a single a

[PATCH] D124441: [Index] [clangd] Support for concept declarations and requires expressions

ilya-biryukov added a comment. In D124441#3474123 , @sammccall wrote: > This looks great! > > Only thing I'm uncomfortable with is the lack of test coverage outside clangd. > I think the most valuable thing would be to have some direct testing of how > R

[PATCH] D124441: [Index] [clangd] Support for concept declarations and requires expressions

ilya-biryukov updated this revision to Diff 425171. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Check 'Concept auto' completion, - set -std=c++20 unconditionally, - use char instead of int. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D124441: [Index] [clangd] Support for concept declarations and requires expressions

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:3584 + std::vector Syms = {conceptSym("same_as")}; + for (auto P : Code.points("tparam")) { +ASSERT_THAT(completions(TU, P, Syms).Completions, sammccal

[PATCH] D124441: [Index] [clangd] Support for concept declarations and requires expressions

ilya-biryukov updated this revision to Diff 425198. ilya-biryukov added a comment. - clang-format the code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124441/new/ https://reviews.llvm.org/D124441 Files: clang-tools-extra/clangd/CodeComplete.cp

[PATCH] D124441: [Index] [clangd] Support for concept declarations and requires expressions

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG75e16fd2c656: [Index] [clangd] Support for concept declarations and requires expressions (authored by ilya-biryukov). Repository: rG LLVM Github M

[PATCH] D124532: [AST] Improve traversal of concept requirements

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added subscribers: usaxena95, kadircet, arphaman. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added projects: clang, clang-tools-extra. - Do not visit implicit AST node t

[PATCH] D124532: [AST] Improve traversal of concept requirements

ilya-biryukov updated this revision to Diff 425713. ilya-biryukov added a comment. - Mention the `AutoType` traversal does not traverse the constrained concept decl anymore and add a test for it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124532/

[PATCH] D124532: [AST] Improve traversal of concept requirements

This revision was automatically updated to reflect the committed changes. Closed by commit rG2b833d4086ab: [AST] Improve traversal of concepts and concept requirements (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124532/

[PATCH] D124606: Use `binary` git attribute instead of `text eol=...'

ilya-biryukov added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124606/new/ https://reviews.llvm.org/D124606 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D124606: Use `binary` git attribute instead of `text eol=...'

ilya-biryukov added a comment. NIT: change the commit message to say `-text` instead of `binary` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124606/new/ https://reviews.llvm.org/D124606 ___ cfe-commits

[PATCH] D124606: Use `-text` git attribute instead of `text eol=...'

ilya-biryukov added a comment. In D124606#3480012 , @aaronpuchert wrote: > I think this change broke again what D97625 > was trying to fix. These are text files and we want them to keep the > specified line endings reg

<    19   20   21   22   23   24   25   26   27   28   >