Details
- Reviewers
rnk majnemer hans - Commits
- rG83665e6d36bc: Re-commit r324991 "Fix for PR32992. Static const classes not exported."
rG3bd0a1586712: Fix for PR32992. Static const classes not exported.
rC326089: Re-commit r324991 "Fix for PR32992. Static const classes not exported."
rL326089: Re-commit r324991 "Fix for PR32992. Static const classes not exported."
rC324991: Fix for PR32992. Static const classes not exported.
rL324991: Fix for PR32992. Static const classes not exported.
Diff Detail
Event Timeline
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? |
lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
5508 | No it doesn't. I removed that condition. |
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. |
Looks good to me. Do you have commit access or would you like me to commit it for you?
I don't see any difference compared to the previous version. What is the fix for the assert?
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? |
lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
5501 | Yes. |
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.
Maybe rename this to ReferenceDllExportedMembers now that it applies to variables as well.