Page MenuHomePhabricator

Fix for Bug45811 - Failed assertion
Needs ReviewPublic

Authored by zahiraam on Thu, May 7, 10:47 AM.

Details

Summary

Compiler is crashing when instantiated default argument marked as not.

Diff Detail

Event Timeline

zahiraam created this revision.Thu, May 7, 10:47 AM
erichkeane added inline comments.Mon, May 18, 8:46 AM
clang/lib/Sema/SemaDeclCXX.cpp
557

What is the MS extension that makes this necessary? Why does the normal inherited default arg not do anything here?

Can you better explain in the commit message what you think is wrong here? Why would this happen only with dll_export, and not the other mechanisms for making 'foo' be emitted?

zahiraam updated this revision to Diff 264701.Mon, May 18, 12:38 PM
zahiraam marked an inline comment as done.
zahiraam added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
557

@erichkeane Thanks for looking at it. Updated the commit message.
The default closure constructor is always in presence of a dllexport, I think.

erichkeane added inline comments.Mon, May 18, 1:15 PM
clang/lib/Sema/SemaDeclCXX.cpp
561

Can MD BE a method-decl and have a parent that isn't a record? That seems the second condition there is ALWAYS true.

566

These seem overly constraining. It also seems like you'd want to check the CXXCtorType of the constructor, to make sure you're dealing with the closure, right?

Can this problem happen in an implicit specialization?

Added @majnemer since he implemented constructor closures in D8331 it looks, I don't think I know enough about them to review.

rnk added a subscriber: hans.Tue, May 19, 11:30 AM

@hans is the resident dllexport expert at this moment.

My really surface-level feedback would be to try to find a way to do this with as few conditions as humanly possible. My mental model is that every added boolean check charges interest, and there are quite a few here, and it's hard for me to understand why they are all needed.

Starting this June, I won't be available for reviews until fall, as a heads up.

zahiraam updated this revision to Diff 265353.Wed, May 20, 2:55 PM
zahiraam added a reviewer: hans.
zahiraam updated this revision to Diff 265856.Sat, May 23, 6:52 AM