[PATCH] D71962: Fix crash in getFullyQualifiedName for inline namespace

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

[PATCH] D71962: Fix crash in getFullyQualifiedName for inline namespace

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

[PATCH] D71982: [docs] Update path to clang-tools-extra

ilya-biryukov added a comment. +1, please upload a diff with full context Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71982/new/ https://reviews.llvm.org/D71982

[PATCH] D71965: include missing for std::abort

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Do you have commit access or should we land this for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71965/new/ https://r

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

ilya-biryukov updated this revision to Diff 235837. ilya-biryukov added a comment. - Use DependencyFlagsBits for computing NumExprBits - Reformat Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 Files: cla

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

ilya-biryukov marked 4 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/Expr.h:126 +if (TD) + D = D | DependencyFlags::Type; +if (VD) Mordante wrote: > Just curious why do you prefer `D = D | Dependenc

[PATCH] D72072: [AST] Respect shouldTraversePostOrder when traversing type locs

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr2. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D72072 Files: clang/include/clang/AST/RecursiveASTVisitor.h Index: clang/include/clang/AST/RecursiveASTVisitor.h

[PATCH] D72072: [AST] Respect shouldTraversePostOrder when traversing type locs

ilya-biryukov added a comment. Would be nice to write a test too, but didn't get to it yet... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72072/new/ https://reviews.llvm.org/D72072 ___ cfe-commits ma

[PATCH] D72073: [Sema] Fix location of star ('*') inside MemberPointerTypeLoc

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr2. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D72073 Files: clang/include/clang/Sema/DeclSpec.h clang/lib/Parse/ParseDecl.cpp clang/lib/Sema/SemaType.cpp Index

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

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/AST.cpp:307 + llvm::raw_string_ostream OS(Result); + auto Decls = explicitReferenceTargets( + ast_type_traits::

[PATCH] D72085: [clangd] Fix hover for functions inside templated classes

ilya-biryukov added a comment. I landed a patch that does exactly this a few days ago: 14e11005d1a6ac1fecb230c470e9011d6956b8e4 Did you pull the latest changes? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

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

ilya-biryukov marked 4 inline comments as done. ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/Expr.h:126 +if (TD) + D = D | DependencyFlags::Type; +if (VD) riccibruno wrote: > ilya-biryukov wrote: > > Mordante wrote: > > >

[PATCH] D72089: [Syntax] Build declarator nodes

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr2. Herald added a project: clang. ilya-biryukov added a parent revision: D72072: [AST] Respect shouldTraversePostOrder when traversing type locs. They cover part of types and names for some declarations, including commo

[PATCH] D72119: [clangd] Handle DeducedTemplateSpecializationType in TargetFinder

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Please add a test for `findExplicitReferences` too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72119/new/ https://revi

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.h:194 +llvm::SmallVector +explicitReferenceTargets(ast_type_traits::DynTypedNode N, + DeclRelationSet Mask = {}); sammccall wrote: > ilya-biryukov wrote:

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.h:194 +llvm::SmallVector +explicitReferenceTargets(ast_type_traits::DynTypedNode N, + DeclRelationSet Mask = {}); sammccall wrote: > ilya-biryukov wrote:

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.h:194 +llvm::SmallVector +explicitReferenceTargets(ast_type_traits::DynTypedNode N, + DeclRelationSet Mask = {}); sammccall wrote: > ilya-biryukov wrote:

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.h:194 +llvm::SmallVector +explicitReferenceTargets(ast_type_traits::DynTypedNode N, + DeclRelationSet Mask = {}); sammccall wrote: > ilya-biryukov wrote:

[PATCH] D72163: [clangd] targetDecl() returns only NamedDecls.

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

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.h:194 +llvm::SmallVector +explicitReferenceTargets(ast_type_traits::DynTypedNode N, + DeclRelationSet Mask = {}); sammccall wrote: > ilya-biryukov wrote:

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/FindTarget.h:194 +llvm::SmallVector +explicitReferenceTargets(ast_type_traits::DynTypedNode N, + DeclRelationSet Mask = {}); ilya-biryukov wrote: > sammccall wrote:

[PATCH] D67224: [clangd] Enable completions with fixes in VSCode

ilya-biryukov added a comment. In D67224#1804513 , @nridge wrote: > Just throwing a wild idea out there: what if we used > `textDocument/onTypeFormatting` to replace the `.` with `->` as soon as it's > typed? There is no way we can do this with proper

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

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:350 + const auto *MainFileEntry = SM.getFileEntryForID(SM.getMainFileID()); + if (!MainFileEntry) +return; Does this ever happen? Maybe `assert` it's not nul

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

ilya-biryukov accepted this revision. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:362 + R.Location.End.setColumn(Range.en

[PATCH] D72334: [Syntax] Build nodes for template declarations.

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr2. Herald added a project: clang. ilya-biryukov added a parent revision: D72089: [Syntax] Build declarator nodes. ilya-biryukov updated this revision to Diff 236583. ilya-biryukov added a comment. - Cosmetics Handles t

[PATCH] D72334: [Syntax] Build nodes for template declarations.

ilya-biryukov updated this revision to Diff 236583. ilya-biryukov added a comment. - Cosmetics Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72334/new/ https://reviews.llvm.org/D72334 Files: clang/include/clang/Tooling/Syntax/Nodes.h clang/lib

[PATCH] D72119: [clangd] Handle DeducedTemplateSpecializationType in TargetFinder

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/FindTarget.cpp:370 +if (auto *TD = DTST->getTemplateName().getAsTemplateDecl()) { + Outer.add(TD->get

[PATCH] D72355: [clangd] Assert that the testcases in FindExplicitReferencesTest.All have no diagnostics

ilya-biryukov requested changes to this revision. ilya-biryukov added inline comments. This revision now requires changes to proceed. Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:571 + ADD_FAILURE() << D << Code; +assert(AST.getDiagnostics().empty()

[PATCH] D72119: [clangd] Handle DeducedTemplateSpecializationType in TargetFinder

ilya-biryukov added a comment. I'm also looking into fixing this in clang, this shouldn't be too hard. But please land the workaround for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72119/new/ https://reviews.llvm.org/D72119 _

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

ilya-biryukov added a comment. @rsmith, gentle ping. WDYT? Is this a step in the right direction? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 ___ cfe-commit

[PATCH] D72442: [Sema] Store deduced type in TypeLoc

ilya-biryukov created this revision. Herald added subscribers: cfe-commits, usaxena95, kadircet. Herald added a project: clang. This greatly simplifies source-level tools that need to know what 'auto' deduced to, e.g. clangd. Also change default presentation of such types to print 'auto' instead

[PATCH] D72442: [Sema] Store deduced type in TypeLoc

ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. This produces the following test failures after running `check-clang`: Failing Tests (4):

[PATCH] D72119: [clangd] Handle DeducedTemplateSpecializationType in TargetFinder

ilya-biryukov added a comment. Here's my attempt at storing the deduced type in `TypeLoc`s: D72442 . It almost worked, but still needs an update to fix some test failures. The change proves it's possible to do this in principle, although it does require a few ha

[PATCH] D72446: [Syntax] Build mapping from AST to syntax tree nodes

ilya-biryukov created this revision. ilya-biryukov added a reviewer: gribozavr2. Herald added a project: clang. ilya-biryukov updated this revision to Diff 237039. ilya-biryukov added a comment. ilya-biryukov added a parent revision: D72334: [Syntax] Build nodes for template declarations.. Remove

[PATCH] D72446: [Syntax] Build mapping from AST to syntax tree nodes

ilya-biryukov updated this revision to Diff 237039. ilya-biryukov added a comment. Remove the (now redundant) NodeAndRole class Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72446/new/ https://reviews.llvm.org/D72446 Files: clang/include/clang/T

[PATCH] D72446: [Syntax] Build mapping from AST to syntax tree nodes

ilya-biryukov updated this revision to Diff 237042. ilya-biryukov added a comment. - Remove debug logs - Cosmetics Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72446/new/ https://reviews.llvm.org/D72446 Files: clang/include/clang/Tooling/Syntax

[PATCH] D72458: [clangd] Adjust diagnostic range to be inside main file

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang-tools-extra/clangd/Diagnostics.cpp:342 +llvm::Optional NoteInsideMainFile; +for (auto &N : D.Notes) { + if (!N.InsideMainFil

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: include/clang/Tooling/Syntax/Tokens.h:206 + /// DECL(a); + /// spelledTokens() returns {"#", "define", "DECL", "(", "name", ")", "eof"}. + /// FIXME: we do not yet store tokens

[PATCH] D72355: [clangd] Assert that the testcases in FindExplicitReferencesTest.All have no diagnostics

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:898 class $10^Foo { - $11^Foo(int); +

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

ilya-biryukov marked 2 inline comments as done. ilya-biryukov added inline comments. Comment at: include/clang/Tooling/Syntax/Tokens.h:206 + /// DECL(a); + /// spelledTokens() returns {"#", "define", "DECL", "(", "name", ")", "eof"}. + /// FIXME: we do not yet store tokens

[PATCH] D72462: [clangd] Fix markdown rendering in VSCode

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

[PATCH] D72494: [clangd] Fix targetDecl() on certain usage of ObjC properties.

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Could you also add a test (and possibly support this in the visitors) for `findExplicitReferences`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72497: [CodeComplete] Suggest 'return nullptr' in functions returning pointers

ilya-biryukov created this revision. ilya-biryukov added a reviewer: kadircet. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D72497 Files: clang/lib/Sema/SemaCodeComplete.cpp clang/test/CodeCompletion/patterns.cpp Index: clang/test/CodeComple

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

ilya-biryukov added a comment. Could it be the case that we want to show the canonical types (i.e. without all syntax sugar)? Maybe we want both the normal type and the canonical type? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72498/new/ https

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

ilya-biryukov added a comment. This does not work for more complicated types, though? E.g. `decltype(a+b)* a` or `vector a`? Why do we prefer to drop `decltype()`, yet show the typedefs? Both could lead to complicated types, arguably decltypes can be even worse than typedefs. Repository: rG

[PATCH] D72497: [CodeComplete] Suggest 'return nullptr' in functions returning pointers

This revision was automatically updated to reflect the committed changes. Closed by commit rG57a51b689e7b: [CodeComplete] Suggest 'return nullptr' in functions returning pointers (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

ilya-biryukov added a comment. In D72498#1813962 , @sammccall wrote: > It's particularly unclear to me why typeprinter descends into auto but prints > decltype, but Kadir says that seems to be intentional. Also don't see why they choose to have this inc

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

ilya-biryukov added a comment. In D72498#1813963 , @kadircet wrote: > I think typedef and decltype have different nature, the latter is a lot more > obscure than the former, that was the reason why I handled decltypes > specifically. I tend to disagree

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

ilya-biryukov added a comment. In D72498#1813962 , @sammccall wrote: > Maybe we want both the normal type and the canonical type? > > Canonical types are often *really* ugly, especially with STL types (we don't > have the "as written" form). And presentin

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

ilya-biryukov added a comment. In D65591#1791876 , @rsmith wrote: > @ilya-biryukov Did I forget anything? SG, I don't think anything is missing. Thanks for the write-up! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

ilya-biryukov added a comment. In D72498#1814366 , @sammccall wrote: > @ilya-biryukov @kadircet what do you think about unwrapping decltype only > when it's a return value (optional: of a function whose leading return type > is auto) to narrowly catch th

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

ilya-biryukov added a comment. In D72498#1815500 , @lh123 wrote: > - hover over the `front` , you'll see "instance-method `front` → > `std::vector >::reference`". > - hover over the `push_back`, you'll see "`std::vector std::allocator >::value_type && __x

[PATCH] D72581: [Syntax] Add mapping from spelled to expanded tokens for TokenBuffer

ilya-biryukov created this revision. ilya-biryukov added a reviewer: sammccall. Herald added a project: clang. Same restrictions apply as in the other direction: macro arguments are not supported yet, only full macro expansions can be mapped. Repository: rG LLVM Github Monorepo https://review

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

ilya-biryukov updated this revision to Diff 237577. ilya-biryukov added a comment. - Add compound assignment operations Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71920/new/ https://reviews.llvm.org/D71920 Files: clang-tools-extra/clang-tidy/

[PATCH] D72498: [clangd] Print underlying type for decltypes in hover

ilya-biryukov added a comment. In D72498#1816424 , @lh123 wrote: > Currently, I think that in most cases, showing both expanded (canonical) and > spelled types is sufficient. > > > This has been used in ycmd for ~4 years without complaint. > > https://gi

[PATCH] D72715: [clang][CodeComplete] Propogate printing policy to FunctionDecl

ilya-biryukov added a comment. The patch contains only tests, are we missing the actual functional change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72715/new/ https://reviews.llvm.org/D72715 ___ c

[PATCH] D72089: [Syntax] Build declarator nodes

ilya-biryukov updated this revision to Diff 238022. ilya-biryukov added a comment. - Rebase, get rid of accidental changes with TemplateDeclaration Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72089/new/ https://reviews.llvm.org/D72089 Files: c

[PATCH] D72334: [Syntax] Build nodes for template declarations.

ilya-biryukov updated this revision to Diff 238023. ilya-biryukov added a comment. - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72334/new/ https://reviews.llvm.org/D72334 Files: clang/include/clang/Tooling/Syntax/Nodes.h clang/lib/To

[PATCH] D136259: Fix crash in constraining partial specialization on nested template.

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5807 bool TraverseTemplateName(TemplateName Template) { -if (auto *TTP = -dyn_cast(Template.getAsTemplateDecl())) - if (TTP->getDepth() == Depth) -Used[TTP->g

[PATCH] D136259: Fix crash in constraining partial specialization on nested template.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136259/new/ https://reviews.llvm.org/D136259

[PATCH] D136248: [Index] consider `delete X` a reference to ~X() if it can be resolved

ilya-biryukov added a comment. Sorry for chiming in, left a comment on GitHub . TLDR; destructors are easy to find and this is not the case for user-defined `operator delete`. Maybe this is a good reason to have `operator delete` references (either

[PATCH] D136440: [clang] Do not hide base member using-decls with different template head.

ilya-biryukov added a comment. Thanks for the change. All the behavior changes look reasonable to me, but I'm struggling to understand whether we follow the standard closely here. Left a comment to discuss this in depth. Comment at: clang/lib/Sema/SemaOverload.cpp:1238 bool S

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

ilya-biryukov added inline comments. Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2179 + for (unsigned I = 0; I < E->NumExprs; I++) +E->getTrailingObjects()[I] = Record.readSubExpr(); +} FYI: I think this is where the crash comes from. We should all

[PATCH] D136440: [clang] Do not hide base member using-decls with different template head.

ilya-biryukov added a comment. My only ask is to add a few more tests (see the relevant comment), otherwise LG. Comment at: clang/lib/Sema/SemaOverload.cpp:1293 +// templates first; the remaining checks follow. +bool SameTemplateParameterList = TemplateParameterListsAre

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

ilya-biryukov added inline comments. Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:2179 + for (unsigned I = 0; I < E->NumExprs; I++) +E->getTrailingObjects()[I] = Record.readSubExpr(); +} ayzhao wrote: > ilya-biryukov wrote: > > FYI: I think this is

[PATCH] D136440: [clang] Do not hide base member using-decls with different template head.

ilya-biryukov accepted this revision. ilya-biryukov added inline comments. This revision is now accepted and ready to land. Comment at: clang/test/SemaTemplate/concepts-using-decl.cpp:174 +} // namespace heads_without_concepts. \ No newline at end of file NIT: a

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

ilya-biryukov added a comment. In D112374#3716982 , @mizvekov wrote: > We even match GCC now: https://gcc.godbolt.org/z/WT93WdE7e > > Ie we are printing the function type as-written correctly now. We don't match GCC in other cases. GCC seems to always pr

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

ilya-biryukov added a comment. I agree that the change in behaviour is reasonable and have no objections to it. The code should not rely on particular output of `__PRETTY_FUNCTION__`. I just wanted to point out that we still don't match GCC in other cases, not that is was a worthwhile goal to ch

[PATCH] D132031: Do not evaluate dependent immediate invocations

ilya-biryukov added a comment. A few NITS for the release notes, otherwise LG Comment at: clang/docs/ReleaseNotes.rst:162 (C++14 [dcl.constexpr]p6 (CWG DR647/CWG DR1358)) +- Correctly defer dependent immediate invocations until template instantiation. + This fixes `Issue 55

[PATCH] D131175: [clangd] Use the "macro" semantic token for pre-defined identifiers

ilya-biryukov added a comment. In D131175#3732379 , @sammccall wrote: > But often I forget and edit it in phab, and I don't know of a command to pull > those edits down into my git repo. I ended up always doing this for landing changes: $ git switch

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

ilya-biryukov added a comment. In D53847#3735704 , @erichkeane wrote: > Note that this would also let us mark P2324 > as complete as well. @ilya-biryukov : Since there is no response, I suspect > the answer here is someo

[PATCH] D132503: Add cxx scope if needed for requires clause.

ilya-biryukov added a subscriber: aaron.ballman. ilya-biryukov added a comment. Thanks for the fix. This looks ok to me, except that I am a bit suspicious of the fact that `DeclaratorScopeObj` is used somewhat rarely. I suspect we might want a different guard class for this, e.g. something simil

[PATCH] D132503: [clang] Add cxx scope if needed for requires clause.

ilya-biryukov added a comment. I will commit this on behalf of Luke. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132503/new/ https://reviews.llvm.org/D132503 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D132503: [clang] Add cxx scope if needed for requires clause.

This revision was automatically updated to reflect the committed changes. Closed by commit rG7aa3270622f4: [clang] Add cxx scope if needed for requires clause. (authored by luken-google, committed by ilya-biryukov). Changed prior to commit: https://reviews.llvm.org/D132503?vs=455622&id=455822#t

[PATCH] D134243: Don't crash when code completing `using enum ^Foo`.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM to fix the crash. If @urnathan's changes happen to make this change obsolete, we could also revert consider reverting it afterwards. Comment at: clang/lib

[PATCH] D134578: Add missing `struct` keyword to the test p2-2.cpp

ilya-biryukov added subscribers: ChuanqiXu, ilya-biryukov. ilya-biryukov added a reviewer: ChuanqiXu. ilya-biryukov added a comment. LGTM. I believe the standard is clear here, the declaration inside `export` does not have any special treatment in terms of how it must be parsed. @ChuanqiXu could

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

ilya-biryukov added a comment. A few ideas for tests: - Static operators. Technically disallowed by the standard, but Clang seems to recover without dropping the member, so it seems we can actually look it up. struct X { bool operator ==(X const&) const; static bool operator !=(X con

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

ilya-biryukov added a reviewer: clang-language-wg. ilya-biryukov added a comment. Overall LG, thanks! The only major comment from me is that we probably want to implement the full "corresponds" check so we handle various cases mentioned in the FIXMEs. Also adding the Language WG as reviewers in

[PATCH] D134578: Add missing `struct` keyword to the test p2-2.cpp

ilya-biryukov added a comment. Is this the only thing that blocks D53847 ? I suggest to stamp this if so (happy to do it myself). In case @ChuanqiXu will have comments we can address them in post-commit review. Repository: rG LLVM Github Monorepo CHANGES SINC

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

ilya-biryukov added a subscriber: rsmith. ilya-biryukov added a comment. In D132941#3826398 , @cor3ntin wrote: > I think this is superseded by > https://cplusplus.github.io/CWG/issues/2631.html and its resolution. > Which I'm looking into implementing -

[PATCH] D119130: [clangd] NFC: Move stdlib headers handling to Clang

ilya-biryukov added a comment. In D119130#3829816 , @thakis wrote: > This makes clang-format depend on clang's AST library Oops, definitely an unintended outcome. Ping @kadircet, in case he missed this. He should know best who can fix this. Repository:

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

ilya-biryukov added a comment. Thanks! I have only various code style comments here, ptal. Overall LG. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4705 +def note_ovl_ambiguous_eqeq_reversed_self_non_const : Note< + "mark operator== as const or add a matching o

[PATCH] D135257: [clangd][Tweak] Make sure enclosing function doesnt have invalid children

ilya-biryukov added a comment. Having `nullptr` inside `children()` seems really weird. Should we fix those instead to never produce `nullptr`? Or is this something that is expected (I can come up with a few contracts where this would make sense, but that all looks really akward). Repository:

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a few minor NITs Comment at: clang/include/clang/Sema/Overload.h:1024 /// candidates for operator Op. - bool shouldAddReversed(OverloadedO

[PATCH] D135257: [clangd][Tweak] Make sure enclosing function doesnt have invalid children

ilya-biryukov added a comment. I don't think it's unreasonable to protect against `nullptr` in individual fields. We need **some** value for it and null is a reasonable choice. However, it feels completely wrong to have `nullptr` in collections. Those should be filtered out upon creation, if pos

[PATCH] D135257: [clangd][Tweak] Make sure enclosing function doesnt have invalid children

ilya-biryukov added a comment. I guess my question is: is there any fundamental reason why we think we **need** to allow `nullptr` children in `Stmt`? What are the places that actually need it? A quick search shows there are quite a few places in our codebase (many google-internal) that don't

[PATCH] D135257: [clangd][Tweak] Make sure enclosing function doesnt have invalid children

ilya-biryukov added a comment. To be clear: landing this change seems fine, I was not trying to block it, sorry if it seemed that way. I'm just trying to understand whether it would also make sense to change the defaults. I believe we are on the same page here, it's just a matter of booking som

[PATCH] D134303: [AST] Preserve more structure in UsingEnumDecl node.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. Modelling this as a `TypeLoc` for code reuse reasons gives me a pause too. However, I think we still accept this as keeping the clients simpler seems like the right trade-off to m

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

ilya-biryukov added a comment. In D134529#3845617 , @sberg wrote: > I just ran into newly-failing > It looks to me like this is correctly rejected now per P2468R2. Yes, this is intentional, the behavior of the code is the same as in C++17. > But it is r

[PATCH] D134529: [C++20][Clang] P2468R2 The Equality Operator You Are Looking For

ilya-biryukov added a comment. In D134529#3852990 , @erichkeane wrote: > Note that @BertalanD noticed an issue with what I expect to be this patch: > https://godbolt.org/z/Wjb9rsEYG > > Can someone more knowledgable about this paper please make sure it

[PATCH] D140327: [clang] Remove overly restrictive aggregate paren init logic

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

[PATCH] D140587: [clang] Fix a clang crash on invalid code in C++20 mode.

ilya-biryukov added subscribers: ayzhao, ilya-biryukov. ilya-biryukov added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:6177 // constructors. For example, conversion function. if (const auto *RD = dyn_cast(DestType->getAs()->getDecl());

[PATCH] D140587: [clang] Fix a clang crash on invalid code in C++20 mode.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM to unbreak clangd, this seems to pop up a lot for Chrome developers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140587/new/

[PATCH] D140547: Perform access checking to private members in simple requirement.

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:1367 + if (E->getBody()->isDependentContext()) { +Sema::SFINAETrap Trap(SemaRef); +// We recreate the RequiresExpr body, but not by instantiating it.

[PATCH] D140876: [clang][C++20] Non-dependent access checks in requires expression.

ilya-biryukov added a comment. Should access checks should happen in the context where `concept` is written or where it's used? Is there a standard wording around it? If access-checking should happen where concept is defined, having a hard error seems fine because of the wording you quoted: > I

[PATCH] D140547: Perform access checking to private members in simple requirement.

ilya-biryukov added a comment. I think the only major problem is not checking for error case when accessing `TransReq`, the rest are NITs. Thanks for the change! Will be happy to LGTM it as soon as the access to `TransReq` is fixed. Comment at: clang/lib/Parse/ParseExprCXX.cp

[PATCH] D140547: Perform access checking to private members in simple requirement.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks for fixing this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140547/new/ https://reviews.llvm.org/D140547

[PATCH] D135859: [Includecleaner] Introduce RefType to ast walking

ilya-biryukov added a comment. Sent out rG453c2879cb2ad6c46267ef8f39f0274aed69d9ee to fix this Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135859/new/ https://reviews.llvm.or

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

ilya-biryukov added a comment. Sorry for not reviewing this sooner. This looks very good overall, but I still have some NITs and a few major comments. For the major points see: - the comment about the filler and the syntactic/semantic form for the newly added expression, - the comment about rel

[PATCH] D137712: Correctly handle Substitution failure in concept specialization.

ilya-biryukov added a comment. We discussed this with Utkarsh offline, he's OOO for some time, so I wanted to leave the summary here. This patch attempts to attack the problem by keeping the information about substitution failure in template arguments of `ConceptSpecializationExpr`. However, it

<    22   23   24   25   26   27   28   29   30   31   >