This is an archive of the discontinued LLVM Phabricator instance.

clang-cl: Only mark dllexported constexpr functions once
AbandonedPublic

Authored by ehsan on Dec 4 2014, 7:49 AM.

Details

Reviewers
hansw
Summary

This fixes PR21718.

Diff Detail

Event Timeline

ehsan updated this revision to Diff 16926.Dec 4 2014, 7:49 AM
ehsan retitled this revision from to clang-cl: Only mark dllexported constexpr functions once.
ehsan updated this object.
ehsan edited the test plan for this revision. (Show Details)
ehsan added a reviewer: hansw.
ehsan added a subscriber: Unknown Object (MLST).
hans added a subscriber: hans.Dec 4 2014, 9:17 AM

Nice!

I think the patch description could be a little more explanatory, maybe something like "don't mark constexpr members of dllexport classes referenced, since they considered are referenced already".

lib/Sema/SemaDeclCXX.cpp
4745

The comment is a little vague (and should end with a period). Maybe something like "constexpr functions are already marked referenced."

Also, David pointed out that this does not only apply to user-defined functions, so it should be moved back to where you first suggested in the PR. Sorry for my misleading suggestion here before.

test/CodeGenCXX/dllexport.cpp
621

Nit: the template is declared as a struct, so it should be referenced as a struct when instantiated too.

Please also add a test where the constructor is explicitly defaulted, i.e.

template <typename> struct P { constexpr P() = default; };
ehsan added inline comments.Dec 4 2014, 10:17 AM
lib/Sema/SemaDeclCXX.cpp
4745
hans added a comment.Dec 4 2014, 10:41 AM

Sorry, it seems I'm reading email in the wrong order today.

lib/Sema/SemaDeclCXX.cpp
4745

Maybe what we should do is something like

if (MD->isReferenced())

continue;

Would that pass all the old tests and your new one?

ehsan added inline comments.Dec 5 2014, 12:57 AM
lib/Sema/SemaDeclCXX.cpp
4745

That doesn't fix the original issue.