This is an archive of the discontinued LLVM Phabricator instance.

Fix for PR32992. Static const classes not exported.
ClosedPublic

Authored by zahiraam on Feb 6 2018, 7:33 AM.

Diff Detail

Event Timeline

zahiraam created this revision.Feb 6 2018, 7:33 AM
rnk added a reviewer: hans.Feb 6 2018, 3:42 PM
hans added a comment.Feb 7 2018, 1:58 AM

Interesting!

lib/Sema/SemaDeclCXX.cpp
5479

Maybe rename this to ReferenceDllExportedMembers now that it applies to variables as well.

5503

Let's move the MD part down until where it's used.

5506

The intendation looks off here. Can you run it through clang-format maybe? It would also be good with a comment explaining why the code is here.

5508

Does the 'const' really make a difference for whether the member should be exported?

test/SemaCXX/export_static_const.cpp
1 ↗(On Diff #132999)

This test doesn't seem to have any "RUN" line, which means nothing actually gets tested.

Can you add a case to test/CodeGenCXX/dllexport.cpp instead?

zahiraam updated this revision to Diff 133498.Feb 8 2018, 2:58 PM
zahiraam marked 5 inline comments as done.
zahiraam added inline comments.
lib/Sema/SemaDeclCXX.cpp
5508

No it doesn't. I removed that condition.

hans added a comment.Feb 9 2018, 12:57 AM

Thanks! Just a few more comments.

lib/Sema/SemaDeclCXX.cpp
5500

Since VD is a class member, I think isDefinedOutsideFunctionOrMethod() is always true and not interesting to check.

test/CodeGenCXX/dllexport.cpp
571 ↗(On Diff #133498)

Can you move this check down where the class etc. are declared below? Maybe also add a comment explaining what it's testing and reference the bug number.

985 ↗(On Diff #133498)

No need for the semicolon in the empty function, here and below. The rest of the file doesn't use this style.

zahiraam updated this revision to Diff 133643.Feb 9 2018, 10:26 AM
zahiraam marked 3 inline comments as done.
hans accepted this revision.Feb 12 2018, 1:03 AM

Looks good to me. Do you have commit access or would you like me to commit it for you?

This revision is now accepted and ready to land.Feb 12 2018, 1:03 AM

Looks good to me. Do you have commit access or would you like me to commit it for you?

Hello.
Please go ahead and to the commit.
Thanks.

This revision was automatically updated to reflect the committed changes.
zahiraam updated this revision to Diff 135135.Feb 20 2018, 1:33 PM

Fixing the assert in the build.

hans added a comment.Feb 21 2018, 2:47 AM

Fixing the assert in the build.

I don't see any difference compared to the previous version. What is the fix for the assert?

zahiraam updated this revision to Diff 135517.Feb 22 2018, 2:14 PM

Added a test case to check the suppression of the warning.

hans added inline comments.Feb 23 2018, 5:27 AM
lib/Sema/SemaDeclCXX.cpp
5501

I wonder if this is the right thing to do..

Would MarkVariableReferenced work instead, similarly to how MarkFunctionReferenced is used for the methods below?

zahiraam updated this revision to Diff 135665.Feb 23 2018, 10:49 AM
zahiraam marked an inline comment as done.
zahiraam added inline comments.
lib/Sema/SemaDeclCXX.cpp
5501

Yes.

hans added a comment.Feb 26 2018, 6:21 AM

I think you only uploaded the diff between this and the previous version. But the S.MarkVariableReferenced version LGTM. Do you want me to commit it for you?

I think you only uploaded the diff between this and the previous version. But the S.MarkVariableReferenced version LGTM. Do you want me to commit it for you?

Yes only the diff. Do you want the entire diff?
If no please go ahead and commit it.
Thanks.

hans added a comment.Feb 26 2018, 7:07 AM

I think you only uploaded the diff between this and the previous version. But the S.MarkVariableReferenced version LGTM. Do you want me to commit it for you?

Yes only the diff. Do you want the entire diff?
If no please go ahead and commit it.
Thanks.

r326089