[PATCH] D141690: [clang] fix consteval ctor code generation assert

ilya-biryukov added a comment. The change seems reasonable. Could you add a test with a crasher repro and some validation of the generated IR? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141690/new/ https://reviews.llvm.org/D141690

[PATCH] D141671: Move definitions to prevent incomplete types.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM! Looks much cleaner now, BTW. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141671/new/ https://reviews.llvm.org/D141671 ___

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

ilya-biryukov added a comment. In D140547#4050752 , @uabelho wrote: > Anyone else see this? I have not checked, but I would not be surprised if we hit the stack size limits with asan enabled @usaxena95, maybe reduce the number of instantiations from

[PATCH] D141671: Move around structs and definitions to prevent incomplete types.

ilya-biryukov added a reviewer: kadircet. ilya-biryukov added a subscriber: kadircet. ilya-biryukov added a comment. There is potentially a way to move less code by keeping all declarations at place and only moving bodies of definitions of constructors and destructors to the `.cpp` file. Not

[PATCH] D141546: [clang] Reland parenthesized aggregate init patches

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, mostly based on the previous review. I have not noticed any significant changes, so have not looked too cautiously. Let me know if there are any particular places that you

[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] 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:

[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: >

[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] 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

[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] 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] D140044: [CodeComplete] Complete members of dependent `auto` variables

ilya-biryukov added inline comments. Comment at: clang/test/CodeCompletion/member-access.cpp:121 // CHECK-DEP-CHAIN: baseTemplateField : [#T#]baseTemplateField -// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:111:41 %s -o - | FileCheck -check-prefix=CHECK-DEP-CHAIN %s

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

ilya-biryukov accepted this revision as: ilya-biryukov. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM! Thanks a lot for driving this to conclusion! Please note that there is one small NIT you may want to fix before landing this. UB definitely explains why

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

ilya-biryukov added a comment. Since the errors only shows up in modular builds, I would look closely for bugs inside `ASTReader`/`ASTWriter`. Also, it seems that `ArrayFiller` is not taken in to account in `computeDependence` and maybe it should be. This assumption is wrong if `ArrayFiller`

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

ilya-biryukov added a comment. This LG, but not accepting as I believe libc++ is still broken if built with modules, right? I have run `check-cxx` locally and it seems to work, but I suspect that's not using modules by default. I have clicked through the links in phrabricator and the errors

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

ilya-biryukov added inline comments. Comment at: clang/include/clang/AST/ExprCXX.h:4824 + const Expr *getArrayFiller() const { +return const_cast(this)->getArrayFiller(); + } NIT: maybe just use `return ArrayFiller`? The code is much simpler and since

[PATCH] D139541: Fix parameter name in Sema::addInitCapture to ByRef.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. BTW, feel free to send small changes like these without review. The common jargon for them in LLVM is NFC (non-functional change) Repository: rG LLVM Github Monorepo

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

ilya-biryukov added a comment. I have sent a follow-up NFC change to fix a typo in the documentation of `--ranking-model=heuristics`. Note that I decided to avoid changing the order of options for `--ranking-model` in the documentation, this didn't seem important. Repository: rG LLVM

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc9b325088d14: [clangd] Allow to build Clangd without decision forest (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES

[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

[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

[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

[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

[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] D139125: [clang] Correctly handle by-reference capture with an initializer that is a pack expansion in lambdas.

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:13156 getSema().buildLambdaInitCaptureInitialization( - C->getLocation(), OldVD->getType()->isReferenceType(), + C->getLocation(), isReferenceType,

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

ilya-biryukov added a comment. This seems to build in both modes, but I still need to figure out what to do with tests with decision forest off. Also, maybe there is a way to minimize the use of preprocessor even more. Comment at:

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

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

[PATCH] D138727: [clang] Skip defaulted functions in zero-as-null-pointer-constant.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM with a few NITs. Thanks! Comment at: clang/lib/Sema/Sema.cpp:600 + // Ignore null pointers in defaulted functions, e.g. defaulted comparison + //

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

ilya-biryukov added a comment. Thanks for the explanation. LGTM! And thanks for adding an assert. It's interesting that recovery for classes seems to be a bit better here: template struct invalid {}; int a = invalid(10); // there is no error:

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

ilya-biryukov added inline comments. Comment at: clang/lib/CodeGen/CGExprAgg.cpp:581 + Expr *filler = nullptr; + if (auto *ILE = dyn_cast(ExprToVisit)) +filler = ILE->getArrayFiller(); ilya-biryukov wrote: > ayzhao wrote: > > ilya-biryukov wrote: > > > -

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

ilya-biryukov added a comment. Thanks for addressing the comments and sorry for a wait before the comments. Please see the comment about syntactic form, I might be holding it wrong, but it looks like we actually loose the syntactic form completely in this case. Comment at:

[PATCH] D138701: [clang-tidy] Ignore cxxRewrittenBinaryOperator in defaulted function decls in modernize-use-nullptr.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGdb335d02a5e7: [clang-tidy] Ignore cxxRewrittenBinaryOperator in defaulted function decls in… (authored by massberg, committed by ilya-biryukov).

[PATCH] D138701: [clang-tidy] Ignore cxxRewrittenBinaryOperator in defaulted function decls in modernize-use-nullptr.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM, thanks! Initially I thought that traversing with `TK_IgnoreUnlessSpelledInSource` might be appropriate. But that idea does not seem to work as we do need to match implicit

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

ilya-biryukov added a comment. E.g. gcc actually accepts the concept definition, but later fails with "wrong number of template arguments". I wonder why Clang has not been doing the latter before this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138210: [clang] Require parameter pack to be last argument in concepts.

ilya-biryukov added a comment. The error itself makes sense, but I don't quite understand why usages of `invalid` did not produce errors before. Any idea why that happened? Are there some other bugs hiding? It seems that at some point when parsing this code: static_assert(invalid also here ;

[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

[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

[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/

[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:

[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 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 =

[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

[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

[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

[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] 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) -

[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

[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

[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

[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

[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. 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

[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

[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).

[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

[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.

[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] 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

[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] 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

[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

[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:

[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] 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:

[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

[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 , +

[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

[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 ) { +IDs.insert(Override.ID); sammccall wrote: > ilya-biryukov wrote: > >

[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 ) { +

[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

[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

[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] 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] 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] 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

[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 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

[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 ,

[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] 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] 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

[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/

[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=455822#toc

[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

[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

[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

[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] 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

[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

[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

[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

[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] 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

[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

[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/

[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

[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

[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

[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`

<    1   2   3   4   5   6   7   8   9   10   >