[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:289 + /// ChangedSettings + void changeConfiguration(std::map ChangedSettings); + Nebiroth wrote: > ilya-biryukov wrote: > > This function is way too general for `ClangdServer`'s interface,

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D39799#920195, @sammccall wrote: > It's doable, but then you're fighting the infrastructure here. The same > ambiguity can exist between any pair of C

[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)

ilya-biryukov added a subscriber: klimek. ilya-biryukov added a comment. I personally think they're useful to have anyway, and they don't add much noise when they're at the end of completions list. I sometimes want to complete an item I know is there and then fix accessibility. Do you think they

[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. This is probably one of the things that I'd like to be configurable. In https://reviews.llvm.org/D39836#920313, @sammccall wrote: > - they bulk up tests and debugging output I'

[PATCH] D39838: [clang] [python] [tests] Update priority values in code completion test

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Can we run those tests as part of `check-all` cmake target or setup a buildbot that runs those? Seems surprising it went unnoticed for so long. Repository: rL LLVM htt

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov created this revision. These preambles are built by ASTUnit and clangd. Previously, preambles were always stored on disk. In-memory preambles are routed back to the compiler as virtual files in a custom VFS. Interface of ASTUnit does not allow to use in-memory preambles, as ASTUnit

[PATCH] D39843: [clangd] Use in-memory preambles in clangd.

ilya-biryukov created this revision. https://reviews.llvm.org/D39843 Files: clangd/ClangdUnit.cpp Index: clangd/ClangdUnit.cpp === --- clangd/ClangdUnit.cpp +++ clangd/ClangdUnit.cpp @@ -235,7 +235,7 @@ // NOTE: we use Buffer.

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added a comment. In https://reviews.llvm.org/D38639#920427, @malaperle wrote: > I'm not sure it's better to use the InclusionDirective callback for this. We > need to get the includes in two places: in the preamble and non-preamble. In > the preamble we use the callback, have to s

[PATCH] D38639: [clangd] #include statements support for Open definition

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:368 std::vector Result; - Resources->getAST().get()->runUnderLock([Pos, &Result, this](ParsedAST *AST) { -if (!AST) - return; -Result = clangd::findDefinitions(*AST, Pos, Logger); - }); +

[PATCH] D39843: [clangd] Use in-memory preambles in clangd.

ilya-biryukov added a comment. After looking at the numbers, I think we should make this configurable. I'll update this change accordingly. Not sure what should be the default, though. I'm currently erring on the side of "in-memory by default". Current implementation eats almost twice as much m

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov updated this revision to Diff 122838. ilya-biryukov marked 2 inline comments as done. ilya-biryukov added a comment. - Fixed comments. - Removed /*ref*/ annotations. - Removed unused "Storage" variable. - Extract a helper function that properly sets up VFS to access the PCHStorage.

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov added inline comments. Comment at: lib/Frontend/ASTUnit.cpp:1028 +IntrusiveRefCntPtr OldVFS = VFS; +Preamble->AddImplicitPreamble(*CCInvocation, /*ref*/ VFS, + OverrideMainBuffer.get()); klimek wrote: > Since

[PATCH] D39843: [clangd] Use in-memory preambles in clangd.

ilya-biryukov updated this revision to Diff 122845. ilya-biryukov added a comment. Made in-memory preambles optional (on-disk by default). https://reviews.llvm.org/D39843 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/Cl

[PATCH] D39843: [clangd] Use in-memory preambles in clangd.

ilya-biryukov added a comment. In https://reviews.llvm.org/D39843#920568, @sammccall wrote: > As a very first step, can we make this configurable but off-by-default? That > will address the pressing diskless-servers need. I think on-disk by default makes sense now until we do the measurements

[PATCH] D39852: [clangd] Support returning a limited number of completion results.

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

[PATCH] D38708: [AST] Flag the typo-corrected nodes for better tooling

ilya-biryukov added a comment. Sorry, not familiar enough with the AST bits yet to LGTM this. Looks good from a higher-level perspective, though. It's a bit of a shame that's it is still super-easy to write code that does not account for typo-corrected nodes. A more explicit approach, similar to

[PATCH] D39430: [clangd] formatting: don't ignore style

ilya-biryukov added inline comments. Comment at: clangd/ClangdLSPServer.cpp:153 + if (ReplacementsOrError) { +C.reply(json::ary{replacementsToEdits(Code, ReplacementsOrError.get())}); + } else { NIT: remove braces from single-statement branches ==

[PATCH] D39430: [clangd] formatting: don't ignore style

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:289 + llvm::Expected> + formatRange(llvm::StringRef Code, PathRef File, Range Rng); + rwols wrote: > ilya-biryukov wrote: > > Why do we accept `Code` as a parameter here instead of getting i

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:206 + std::unique_ptr Storage; + if (InMemStorage) { +OS = llvm::make_unique(*InMemStorage); klimek wrote: > ilya-biryukov wrote: > > klimek wrote: > > > It looks like we

[PATCH] D39843: [clangd] Use in-memory preambles in clangd.

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:238 if (Preamble) { -Preamble->AddImplicitPreamble(*CI, Buffer.get()); +Preamble->AddImplicitPreamble(*CI, /*ref*/ VFS, Buffer.get()); } else { sammccall wrote: > ref is still he

[PATCH] D39843: [clangd] Use in-memory preambles in clangd.

ilya-biryukov updated this revision to Diff 123035. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - Removed /*ref*/. - Changed the command-line flag: it's -pch-storage now instead of -in-memory-pchs. - Actually pass the StoreInMemory flag to PrecompiledPreamble::B

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:44 +SmallString<64> Path; +llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path); +llvm::sys::path::append(Path, "___clang_inmemory_preamble___"); kl

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.cpp:580 +void ClangdServer::reparseOpenedFiles() { + for (auto Draft : DraftMgr.getDrafts().keys()) { +forceReparse(Draft); Could we have a method in `DraftStore` that returns all active fi

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:44 +SmallString<64> Path; +llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path); +llvm::sys::path::append(Path, "___clang_inmemory_preamble___"); kl

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:44 +SmallString<64> Path; +llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path); +llvm::sys::path::append(Path, "___clang_inmemory_preamble___"); kl

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov updated this revision to Diff 123171. ilya-biryukov added a comment. - Use a hard-coded virtual path for in-memory PCHs instead of system_temp_directory. https://reviews.llvm.org/D39842 Files: include/clang/Frontend/FrontendActions.h include/clang/Frontend/PrecompiledPreamble

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov updated this revision to Diff 123172. ilya-biryukov added a comment. - Removed redundant #include. https://reviews.llvm.org/D39842 Files: include/clang/Frontend/FrontendActions.h include/clang/Frontend/PrecompiledPreamble.h lib/Frontend/ASTUnit.cpp lib/Frontend/FrontendAct

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:44 +SmallString<64> Path; +llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path); +llvm::sys::path::append(Path, "___clang_inmemory_preamble___"); il

[PATCH] D39571: [clangd] DidChangeConfiguration Notification

ilya-biryukov added inline comments. Comment at: clangd/Protocol.h:295 + +struct ClangdConfigurationParams { + malaperle wrote: > ilya-biryukov wrote: > > Maybe call it `ClangdConfigurationParamsChange` to make it clear those are > > diffs, not the actual params

[PATCH] D39842: Allow to store precompiled preambles in memory.

ilya-biryukov added a comment. In https://reviews.llvm.org/D39842#927578, @mgorny wrote: > Please revert this commit. You've just broken all the stand-alone builds of > clang. Sorry about that. Should be fixed in https://reviews.llvm.org/rL318514 Repository: rL LLVM https://reviews.llvm.o

[PATCH] D20124: [PCH] Serialize skipped preprocessor ranges

ilya-biryukov added a comment. In https://reviews.llvm.org/D20124#927967, @cameron314 wrote: > Well, it seems like preamble PCH source location translation is fundamentally > broken. The entry file has a single, positive file ID. The preamble PCH is > treated as an imported module, so it has a

[PATCH] D40185: Loosen -Wempty-body warning.

ilya-biryukov created this revision. Do not show it when `if` or `else` come from macros. E.g., #define USED(A) if (A); else #define SOME_IF(A) if (A) void test() { // No warnings are shown in those cases now. USED(0); SOME_IF(0); } https://reviews.llvm.org/D40185 Files:

[PATCH] D40185: Loosen -Wempty-body warning.

ilya-biryukov added a comment. In https://reviews.llvm.org/D40185#928845, @rnk wrote: > Why does passing the rparen location for the if condition fix the warning for > IF_ELSE? I assumed that would refer to the else clause semicolon. Using spelling locations fixes the `else` case, but breaks s

[PATCH] D39430: [clangd] formatting: don't ignore style

ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:289 + llvm::Expected> + formatRange(llvm::StringRef Code, PathRef File, Range Rng); + rwols wrote: > rwols wrote: > > ilya-biryukov wrote: > > > rwols wrote: > > > > ilya-biryukov wrote: > >

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

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. I haven't looked into the `getHover` method's body yet, but here are some comments about other parts. Good work, BTW, this looks close to being ready. ===

[PATCH] D40301: [clangd] Ensure preamble outlives the AST

ilya-biryukov created this revision. In-memory preambles will not be copied anymore, so we need to make sure they outlive the AST. https://reviews.llvm.org/D40301 Files: clangd/ClangdUnit.cpp clangd/ClangdUnit.h Index: clangd/ClangdUnit.h ===

[PATCH] D40302: Avoid copying the data of in-memory preambles

ilya-biryukov created this revision. Preambles are large and we should prefer not to copy it. https://reviews.llvm.org/D40302 Files: include/clang/Frontend/PrecompiledPreamble.h lib/Frontend/PrecompiledPreamble.cpp Index: lib/Frontend/PrecompiledPreamble.cpp ==

[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)

ilya-biryukov added inline comments. Comment at: clangd/tool/ClangdMain.cpp:169 + clangd::CodeCompleteOptions CCOpts; + CCOpts.EnableSnippets = EnableSnippets; + CCOpts.IncludeIneligibleResults = IncludeIneligibleResults; We should also set `IncludeCodePattern

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

ilya-biryukov added a comment. Reverting the RVO breaks some coroutine code we have. The short repro is https://gcc.godbolt.org/z/1543qc8Ee (code at the end of the post, very similar to the deleted constructor in `warn-throw-out-noexcept-coro.cpp`): The RVO seems to be mandated by the standard

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

ilya-biryukov added a comment. In D145639#4198794 , @ChuanqiXu wrote: > Reverting is common in Clang/LLVM. But let's wait for the response from > @bruno temporarily. I think it makes sense to revert this one if @bruno don't > response tomorrow. And we c

[PATCH] D145639: [Coroutines] Fix premature conversion of return object

ilya-biryukov added a comment. I suspect D145641 won't land today in the EU working hours. My suggestion would be to revert this patch and reland together with D145641 . This should be in line with @ChuanqiXu's suggestions to w

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Currently, Clang stores `nullptr` in the parameter lists inside `Funct

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov added a comment. In D146426#4206519 , @shafik wrote: > Please edit the summary to indicate this fixes: > https://github.com/llvm/llvm-project/issues/61441 > I am happy someone had more time to dig into this problem after my initial > inves

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov updated this revision to Diff 506623. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. - rename test file to GH61441.cpp, add a comment this checks for no crash - Update outdated comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1714 /// Check whether a function's parameter types are all literal types. If so, /// return true. If not, produce a suitable diagnostic and return false. static bool CheckConstexprParameterTypes(S

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov added a comment. In D146426#4207118 , @aaron.ballman wrote: > This feels like it's heading in the wrong direction -- the AST should not > have holes in it. An invalid type should be replaced by a valid type (after > diagnosing the invalid

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov added a comment. In D146426#4207423 , @shafik wrote: > As I noted in the bug report not doing `D.setInvalidType();` does fix this > bug and seems harmless since the error diagnostic should prevent us from > getting to codegen but it is not

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov updated this revision to Diff 506934. ilya-biryukov added a comment. - Add a test for block pointers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146426/new/ https://reviews.llvm.org/D146426 Files: clang/lib/Sema/SemaChecking.cpp

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG282cae0b9a60: [Sema] Fix crash on __fp16 parameters in template instantiations (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo C

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov added a comment. I landed the fix to unbreak our crashes and explored a bit of the alternative solution. Digging a bit deeper, trying to always set non-null parameters causes ~30 test failures, but the ones I looked at so far look more localized and should be fixable. Some starte

[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

ilya-biryukov created this revision. ilya-biryukov added reviewers: aaron.ballman, erichkeane. Herald added a subscriber: arphaman. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This also reverts

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

ilya-biryukov added a comment. In D146426#4209620 , @aaron.ballman wrote: > Thank you for offering to do that in a follow-up, but please, next time wait > for there to be agreement on the patch before landing it. Multiple reviewers > expressed concerns

[PATCH] D146634: [clang][USR] Prevent crashes when parameter lists have nulls

ilya-biryukov added a comment. Hopefully this should be addressed by D146971 , but it would be great to get a reproducer for this issue to be ceratin. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146634/new/ htt

[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

ilya-biryukov added a comment. In D146971#4224465 , @erichkeane wrote: > One other thing we probably should do is have an assert when creating a > function type that none of its params are null. WDYT? This would definitely be great, however I don't th

[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

ilya-biryukov added a comment. In D146971#4224540 , @erichkeane wrote: > Ah, hrmph. I guess I was just hoping for some assert (perhaps even in > `GetFullTypeForDeclarator`?) to assert in order to give future folks a hint > as to why their change crashe

[PATCH] D145369: Emit const globals with constexpr destructor as constant LLVM values

ilya-biryukov added a comment. I don't have a lot of experience in codegen, so will let Aaron and Richard do the review. However, still wanted to share one observation. The actual check that avoids emitting the destructors for variables seems more involved than just checking if the destructor

[PATCH] D153294: [clang] Do not create ExprWithCleanups while checking immediate invocation

ilya-biryukov added a comment. As mentioned in the GH issue, I think this change looks fine. But I would suggest waiting for feedback from @rsmith to ensure there isn't a reason for cleanups being removed that we are missing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D153294: [clang] Do not create ExprWithCleanups while checking immediate invocation

ilya-biryukov added a comment. In D153294#4435767 , @rsmith wrote: > A constant expression (including the immediate invocations generated for > `consteval` functions) is a full-expression, so destructors should be run at > the end of evaluating it, not

[PATCH] D153294: [clang] Do not create ExprWithCleanups while checking immediate invocation

ilya-biryukov added a comment. In D153294#4437381 , @Fznamznon wrote: > In fact, I can't come up with the test that this patch would break. Either > `ExprWithCleanups` is redundant or added by different parts of Sema. Same here, maybe @rsmith can come u

[PATCH] D153294: [clang] Do not create ExprWithCleanups while checking immediate invocation

ilya-biryukov added a comment. While we wait for Richard's response... @Fznamznon what are your thoughts on wrapping all `ConstantExpr` that span immediate invocations with redundant `ExprWithCleanups` per Richard's suggestions? I would be comfortable LGTMing such a change even if we choose to r

[PATCH] D153294: [clang] Do not create ExprWithCleanups while checking immediate invocation

ilya-biryukov added a comment. In D153294#065 , @Fznamznon wrote: >> While we wait for Richard's response... @Fznamznon what are your thoughts on >> wrapping all ConstantExpr that span immediate invocations with redundant >> ExprWithCleanups per Ric

[PATCH] D153294: [clang] Do not create ExprWithCleanups while checking immediate invocation

ilya-biryukov added a comment. In D153294#4448342 , @Fznamznon wrote: > I'm having a slight trouble with understanding why this part is required and > how to implement the test ... > > Also, simply adding a flag to `MaybeCreateExprWithCleanup` signaling

[PATCH] D153962: [clang] Do not discard cleanups while processing immediate invocation

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:7374 auto *E = ExprWithCleanups::Create( Context, SubExpr, Cleanup.cleanupsHaveSideEffects(), Cleanups); Because we may potentially pass some cleanups here (which are outs

[PATCH] D153962: [clang] Do not discard cleanups while processing immediate invocation

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with two NITs. Thanks a lot for fixing this! Comment at: clang/lib/Sema/SemaExpr.cpp:18187 + if (Cleanup.exprNeedsCleanups()) { +// Since an immediate

[PATCH] D153962: [clang] Do not discard cleanups while processing immediate invocation

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM once again with one NIT about the comment line placement. Thanks! Comment at: clang/lib/Sema/SemaExpr.cpp:18198-18199 +// - blocks are not allowed inside constant expressions and compiler will +//

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

ilya-biryukov added a comment. This breaks the following innocent looking out-of-line definition with two levels of constrained template parameters: template concept Result = true; template class CoFuture final { template explicit CoFuture(); }; template template CoF

[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

ilya-biryukov updated this revision to Diff 511462. ilya-biryukov added a comment. - Add assertions for removed null checks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146971/new/ https://reviews.llvm.org/D146971 Files: clang/lib/Sema/SemaChec

[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

ilya-biryukov updated this revision to Diff 511463. ilya-biryukov added a comment. - Another forgotten non-null assertion Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146971/new/ https://reviews.llvm.org/D146971 Files: clang/lib/Sema/SemaChecki

[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

ilya-biryukov added a comment. Sorry for taking so long to land this, it fell off my radar. In D146971#4227482 , @aaron.ballman wrote: > LGTM, though the change should come with a release note. Suggestion you can > take or leave as you see fit: should

[PATCH] D146971: [Sema] Populate declarations inside TypeLocs for some invalid types

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf27b77e5c59a: [Sema] Populate declarations inside TypeLocs for some invalid types (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo

[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

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. Herald added a subscriber: cfe-commits. Classes can have implicit members that were added before fields were

[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

ilya-biryukov added a comment. I plan to land this together with a reland of bc95f27337c7ed77c28e713c855272848f01802a and finally close GH61065 . I have checked this works on

cfe-commits@lists.llvm.org

ilya-biryukov added a comment. @massberg did we figure out if there is anything else left from P2002R1? Are there blockers to landing this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148924/new/ https://reviews.llvm.org/D148924 ___

cfe-commits@lists.llvm.org

ilya-biryukov added a comment. In D148924#4320694 , @shafik wrote: > The paper also uses the term constexpr compatible which has been replaced > with constexpr suitable and not sure if that has any practical effect here. They seem to be different term,

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

ilya-biryukov added a comment. Another friendly ping for review. @erichkeane let me know if you feel that exposing the incorrect lambda instantiation behavior is a blocker here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148802/new/ https://rev

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

ilya-biryukov updated this revision to Diff 520628. ilya-biryukov added a comment. - Moved the relase note to 'C++20 feature support', fixed typos Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148802/new/ https://reviews.llvm.org/D148802 Files:

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

ilya-biryukov updated this revision to Diff 520630. ilya-biryukov added a comment. - Fix a typo (forgot to commit last time) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148802/new/ https://reviews.llvm.org/D148802 Files: clang/docs/ReleaseNote

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

ilya-biryukov added a comment. Thanks! This looks like a useful addition. Could you add a test into `libIndex` for this too? Just for the sake of having better coverage, some folks probably don't run `clang-tools-extra` tests. Also, could we add a test that a label does not get indexed if local

[PATCH] D150124: [index][clangd] Consider labels when indexing function bodies

ilya-biryukov added a reviewer: kadircet. ilya-biryukov added a comment. In `clangd` we also have `findExplicitReferences` and `targetDecl` functions that seem to handle the `GoToStmt`. In fact, I believe they are used in `findDocumentHighlights`, so I'm not sure why your test did not work befor

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

This revision was automatically updated to reflect the committed changes. Closed by commit rG629170fe452f: [Sema] Lambdas are not part of immediate context for deduction (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D14880

[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

ilya-biryukov updated this revision to Diff 514579. ilya-biryukov added a comment. - clang-format the code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148515/new/ https://reviews.llvm.org/D148515 Files: clang/lib/AST/Decl.cpp clang/test/Modu

[PATCH] D148515: [Modules] Do not rewrite existing decls when deserializing class fields

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGccf719171937: [Modules] Do not rewrite existing decls when deserializing class fields (authored by ilya-biryukov). Repository: rG LLVM Github Mono

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

ilya-biryukov created this revision. ilya-biryukov added reviewers: aaron.ballman, erichkeane. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This commit implements [temp.deduct]p9. Test updates i

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:965 +case CodeSynthesisContext::LambdaExpressionSubstitution: + // FIXME: add a note for lambdas. break; erichkeane wrote: > Would really like this note he

cfe-commits@lists.llvm.org

ilya-biryukov added a reviewer: clang-language-wg. ilya-biryukov added a comment. Thanks! I believe we should also recover in the similar manner we do for missing `const`, see corresponding comment. Extra NITs: - there is a typo in description: *compariosn* should be comparison, - we probably w

[PATCH] D148663: [RFC][clangd] Use interpolation for CDB pushed via LSP protocol

ilya-biryukov added a comment. I wanted to chime in and provide a bit of context. This was a long time ago, so I might misremember, so take this with a grain of salt. Idea behind pushing the CDB over LSP was to allow the capable client to **fully** control the commands produced for the files. D

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

ilya-biryukov updated this revision to Diff 517550. ilya-biryukov marked 5 inline comments as done. ilya-biryukov added a comment. - Add a note when substituting into a lambda - Quote the standard and add explanation for the test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

ilya-biryukov added a comment. In D148802#4283566 , @erichkeane wrote: > My one concern is that this is going to expose our incorrect instantiation of > lambdas further and more painfully (see > https://github.com/llvm/llvm-project/issues/58872). Else

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

ilya-biryukov added a comment. > We would like to move the preamble index out of the critical path Could you provide motivation why you need to do it? What is the underlying problem that this change is trying to solve? We rely on preamble being indexed at that particular time for correct results

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

ilya-biryukov added a comment. In D148088#4302182 , @kuganv wrote: > 1. We see preamble indexing taking as much as 18% of the time for some files. > Moving preamble indexing out of the critical path helps there. Which operation are you measuring? 18%

[PATCH] D148663: [RFC][clangd] Use interpolation for CDB pushed via LSP protocol

ilya-biryukov added a comment. In D148663#4301589 , @DmitryPolukhin wrote: > And, if they do so, this diff will just does nothing because there is an > exact match. It starts playing only if client provided partial CDB and > inference is required. (hy

[PATCH] D149733: [clang][USR] Prevent crashes on incomplete FunctionDecls

ilya-biryukov added a subscriber: erichkeane. ilya-biryukov added a comment. It's true that `FunctionType` having a null type does break a lot of even its own methods, e.g. even something as simple as `isVariadic` will dereference a null pointer as it uses `getType()->getAs()`. I am not entirel

[PATCH] D148802: [Sema] Lambdas are not part of immediate context for deduction

ilya-biryukov added a comment. Friendly ping for another round of review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148802/new/ https://reviews.llvm.org/D148802 ___ cfe-commits mailing list cfe-commi

cfe-commits@lists.llvm.org

ilya-biryukov accepted this revision as: ilya-biryukov. ilya-biryukov added a comment. This revision is now accepted and ready to land. Since there are not remaining blockers, LGTM from my side. I suggest waiting for ~day to give chance to other reviewers. @shafik, @aaron.ballman, @rsmith please

[PATCH] D152747: [include-cleaner] Traverse implicit template instantations if the templates are inside the main file.

ilya-biryukov added a comment. Though I'm not a reviewer, I was looking through the emails and found this change interesting. This is not intended to block the review in any way, just trying to get a picture of the direction include-cleaner is taking. Is this a digression from the initial idea

[PATCH] D139107: [clangd] Allow to build Clangd without decision forest

ilya-biryukov updated this revision to Diff 480034. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Merge DecisionForest.cpp and *_disabled.cpp, change #ifndef to #if - Add decision_forest to the feature string - Set Heuristics as the default code completion model

[PATCH] D139107: [clangd] Allow to build Clangd without decision forest

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/Features.inc.in:7 #define CLANGD_TIDY_CHECKS @CLANGD_TIDY_CHECKS@ +#define CLANGD_DECISION_FOREST @CLANGD_DECISION_FOREST@ sammccall wrote: > we **could** include this in the feature-strin

[PATCH] D129531: [clang][C++20] P0960R3 and P1975R0: Allow initializing aggregates from a parenthesized list of values

ilya-biryukov added a comment. Thanks! I have two major comments and also inline NITs. Not sure if we should block on those, just wanted to hear your opinions: - `InitListExpr` and `CXXParenInitListExpr` have some code in common. Code duplication is substantial and I think sharing the common i

[PATCH] D139125: [clang] Correctly handle by-reference capture with an initializer that is a pack expansion in lambdas.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Thanks, LGTM! Comment at: clang/include/clang/Sema/Sema.h:7101 + void addInitCapture(sema::LambdaScopeInfo *LSI, VarDecl *Var, + bool isRe

[PATCH] D139107: [clangd] Allow to build Clangd without decision forest

ilya-biryukov updated this revision to Diff 480857. ilya-biryukov marked 3 inline comments as done. ilya-biryukov added a comment. - Add a comment to CMakelists.txt explaining motivation for the option - Add #include "Feature.h" - Revert changes to the order of compiler flags to keep this change m

<    26   27   28   29   30   31   32   33   >