[PATCH] D159351: [Sema] Change order of displayed overloads in diagnostics

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGfebf5c97bba7: [Sema] Change order of displayed overloads in diagnostics (authored by ilya-biryukov). Changed prior to commit:

[PATCH] D159351: [Sema] Change order of displayed overloads in diagnostics

ilya-biryukov updated this revision to Diff 557839. ilya-biryukov marked 4 inline comments as done. ilya-biryukov added a comment. - Addressed comments - Added a release note Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159351/new/

[PATCH] D159351: [Sema] Change order of displayed overloads in diagnostics

ilya-biryukov added a comment. In D159351#4653687 , @shafik wrote: > You say "attempts to be a strict weak order" does that mean there are still > cases which will cause an assert or are we not sure? No, it's an actual strict weak order. It's a bad

[PATCH] D159351: [Sema] Change order of displayed overloads in diagnostics

ilya-biryukov added a comment. @cor3ntin any suggestions on how to proceed here? I hope the approach taken in the current patch should already provide meaningful and improved results in most cases. It only affects the diagnostics output, so changing or reverting it should be relatively easy in

[PATCH] D159351: [Sema] Change order of displayed overloads in diagnostics

ilya-biryukov added reviewers: rnk, clang-language-wg. ilya-biryukov added a subscriber: rnk. ilya-biryukov added a comment. This change mainly tried to address the assertion failure in libc++, but got into the territory of changing the order of displayed overloads. Which might be a more

[PATCH] D159351: [Sema] Change order of displayed overloads in diagnostics

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. Make it a strict weak order. Fixes #64121. Current implementation uses the definition the definition of

[PATCH] D158985: [AST] Fix crash in evaluation of OpaqueExpr in Clangd

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd703dcdfb08d: [AST] Fix crash in evaluation of OpaqueExpr in Clangd (authored by ilya-biryukov). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D158985: [AST] Fix crash in evaluation of OpaqueExpr in Clangd

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

[PATCH] D158967: [clangd] Record the stack bottom before building AST

ilya-biryukov added a comment. In D158967#4620931 , @zyounan wrote: > We had already placed the initialization in ASTFrontendAction::ExecuteAction >

[PATCH] D158967: [clangd] Record the stack bottom before building AST

ilya-biryukov added a comment. Would it be better to do this in the code that runs the parser? This should help not only Clangd, but all the tools as well. `clang::ParseAST` or `ASTFrontendAction::ExecuteAction` look like good candidates to me. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D158566: Add CLANGD_INCLUDE_TESTS as a separate flag to control clangd tests

ilya-biryukov requested changes to this revision. ilya-biryukov added a subscriber: sammccall. ilya-biryukov added a comment. This revision now requires changes to proceed. Open question: I also feel like the best option here is to fix the tests, but I'm not sure how hard that would be.

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

ilya-biryukov abandoned this revision. ilya-biryukov added a comment. Thanks for clarifying! I misread the standard. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157708/new/ https://reviews.llvm.org/D157708

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

ilya-biryukov added a reviewer: clang-language-wg. ilya-biryukov added a comment. @shafik, @rsmith friendly ping for a review. Also adding clang-language-wg for a wider set of potential reviewers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D158269: [clang] Prevent possible use-after-free

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM. Thanks for getting to the bottom of this! Comment at: clang/lib/Parse/ParseObjc.cpp:3768 + // Clean up the remaining EOF token, only if it's inserted by

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:967 S.getScopeForContext(EqFD->getEnclosingNamespaceContext())); NonMembers.suppressAccessDiagnostics(); for (NamedDecl *Op : NonMembers) { I believe we should

[PATCH] D155387: [Clang] Fix member lookup so that we don't ignore ambiguous lookups in some cases

ilya-biryukov added a comment. This change seems to expose a bug in Clang in C++20 see godbolt . I believe following code should compile, but it does not: struct X { virtual ~X(); virtual bool operator ==(X); bool operator !=(X); };

[PATCH] D157708: [Sema] Suppress lookup diagnostics when checking reversed operators

ilya-biryukov created this revision. ilya-biryukov added reviewers: shafik, rsmith. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The standard specified that Clang should 'search' for members in

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ilya-biryukov added a comment. In D153114#4571703 , @ChuanqiXu wrote: > For example, may it be a reasonable expectation that we can have named > modules support in clangd in clang18? Clang18 will be released in the first > week of March 2024. So that's

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ilya-biryukov added a comment. In D153114#4569591 , @ChuanqiXu wrote: > BTW, I have a question about supporting header modules (including clang > header modules and C++20 header units) in static analysing tools (including > clangd), "why can't we

[PATCH] D153114: [clangd] [C++20] [Modules] Support C++20 modules for clangd

ilya-biryukov requested changes to this revision. ilya-biryukov added a comment. This revision now requires changes to proceed. Hi @ChuanqiXu, Sam is on vacation now, but we are in the same team and I am responding on behalf of the team. First, sorry for not getting to this review earlier. The

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

ilya-biryukov added a comment. If we don't find existing buildbots owners with this, I can volunteer to add a new one with hardened libc++. I have set up the C++20 buildbot last year and getting a new one running over the same infrastructure should be easy. Repository: rG LLVM Github

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov abandoned this revision. ilya-biryukov added a comment. Marking the revision as abandoned to reflect the status of the RFC. Thanks everyone for your feedback and sorry about the timing of this change again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. In D156247#4542155 , @aaron.ballman wrote: > Given the views expressed on this thread, I think this requires a wider > community discussion to determine whether we want to support this idea or > not, regardless of -cc1

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. In D156247#4541826 , @cor3ntin wrote: > How does a -cc1 flag alleviate the burden concerns for libc++? It would still > be something they would have to support, unless we plan to remove the flag > before c++26 and you are

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added subscribers: thieta, tstellar. ilya-biryukov added a comment. @aaron.ballman the internal `-cc1` flag is good enough for us and should aleviate most of the concerns in this thread. We could also live without a libc++ change. I suggest to move forward with `-cc1` flag. Would

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. In D156247#4538962 , @philnik wrote: > I don't think we want to support having another flag for a dialect, since > that will result in more problems down the road. We already have a problem > with the number of

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov updated this revision to Diff 544783. ilya-biryukov added a comment. Herald added a project: clang-format. Herald added reviewers: rymiel, HazardyKnusperkeks, owenpan, MyDeveloperDay. - Expose -fno-coroutine and add tests Current implementation exposes: - `-fno-coroutines` to

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a subscriber: ldionne. ilya-biryukov added a comment. In D156247#4538647 , @cor3ntin wrote: > Given that adoption by Apple Clang is one of the motivation to make this > change upstream, do we have a reasonable expectation they would

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. Thanks, disabling the language feature definitely makes more sense. `-fno-coroutines` is trickier to implement, but this cannot be too hard. While we are waiting for libc++ folks to come back with feedback, what would be the desired behavior. GCC seems to 1.

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. In D156247#4535664 , @tahonermann wrote: > At first, it sounded to me like the option that would best suit Google's > needs is `-fno-coroutines`. However: > > - I see that Google does have some limited use of coroutines;

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. The list of bugs I shared was meant to illustrate the problem, it's not meant to be exhaustive. I am not sure there is a need to rush and fix them before the release, it is just a signal that makes us worried about the feature. The bugs are niche, and likely not a

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. Thanks for the quick feedback! I understand that this goes against common practice in Clang, but the reason we want this warning is very practical. At Google, we would like to postpone adoption of coroutines until the aforementioned bugs are fixed, but we feel

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov added a comment. I know this is a little late and we should have thought about it before cutting Clang 17 release. The plan is to cherry-pick this change into Clang 17 after it lands. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D156247: [Clang] Add a warning on uses of coroutine keywords

ilya-biryukov created this revision. Herald added a subscriber: ChuanqiXu. Herald added a project: All. ilya-biryukov requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Clang 17 will land with unaddressed coroutine bugs, see -

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

[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

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

[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

[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

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

[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] D148924: [clang] Show error if defaulted comparions operator function is volatile or has ref-qualifier &&.

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] D148924: [clang] Show error if defaulted comparions operator function is volatile or has ref-qualifier &&.

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

[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

[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

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

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

[PATCH] D148924: [clang] Show error if defaulted comparions operator function is volatile or has ref-qualifier &&.

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

[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

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

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

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

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

[PATCH] D148924: [clang] Show error if defaulted comparions operator function is volatile or has ref-qualifier &&.

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

[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

[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

[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

[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

[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

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

[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

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:

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

[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

[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

[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

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

[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

[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

[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

[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

[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

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

[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

[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

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

[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

[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

[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

[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] 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] D142384: [C++20] Fix a crash with modules.

ilya-biryukov added a comment. In D142384#4101461 , @ChuanqiXu wrote: > It should be good to prevent crashes. But it looks not good that it doesn't > have a test. Do you have plans to add a test case for this soon? It would be great to have a test, but

[PATCH] D142384: [C++20] Fix a crash with modules.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. In D142384#4098650 , @usaxena95 wrote: > I think this is fine, and we should just use the definition when it is > available without

[PATCH] D142384: [C++20] Fix a crash with modules.

ilya-biryukov added a comment. I don't see any issues with this, so happy to LGTM. @rsmith any concerns on your side? @usaxena95 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142384/new/ https://reviews.llvm.org/D142384

[PATCH] D142384: [C++20] Fix a crash with modules.

ilya-biryukov added a reviewer: rsmith. ilya-biryukov added a comment. This fix looks very sensible to me and @rsmith confirmed this pattern that we are seeing might happen (seeing members of something that was demoted from declaration to a definition). @rsmith could you confirm the update

[PATCH] D142187: [clang] Fix typos in member initializers

ilya-biryukov added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:4101 - ExprResult Init = InitExpr; - if (!FD->getType()->isDependentType() && !InitExpr->isTypeDependent()) { -Init = ConvertMemberDefaultInitExpression(FD, InitExpr, InitLoc); + ExprResult

[PATCH] D142187: [clang] Fix typos in member initializers

ilya-biryukov added a comment. Randomly chiming in here. I never had a good model of where `CorrectDelayedTyposInExpr`, but wanted to note that `ActOnFullExpr` also calls it. This may be fine, I just wanted to mention it as it stood out. Comment at:

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

ilya-biryukov added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:1338 + llvm::StructType *STy = dyn_cast(Val->getType()); + if (STy && STy->canLosslesslyBitCastTo(Dest.getElementType())) { for (unsigned i = 0, e = STy->getNumElements(); i != e; ++i) {

[PATCH] D141818: Add test for an invalid requirement in requires expr.

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

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

ilya-biryukov added a subscriber: jyknight. ilya-biryukov added a comment. Sorry, didn't see your comment when submitting mine. I think the offending code is clearly wrong here and adding the `isLayoutIdentical` seems like the right thing to do. This looks like a small change and I'm happy to

  1   2   3   4   5   6   7   8   9   10   >