[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman closed this revision. aaron.ballman added a comment. > Would you mind committing the changes please? Thanks. Happy to do so! I've committed in r356458. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added a comment. In D45978#1431057 , @aaron.ballman wrote: > LGTM! Would you mind committing the changes please? Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 190124. zahiraam marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: lib/Sema/SemaDecl.cpp test/CodeGen/dllexport-1.c test/Sema/dllexport-1.cpp test/Sema/dllexport

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam marked 6 inline comments as done. zahiraam added inline comments. Comment at: test/SemaCXX/dllexport.cpp:72-74 +#ifndef MS namespace{ __declspec(dllexport) int InternalGlobal; } // expected-error{{'(anonymous namespace)::InternalGlobal' must have external linkage

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. I think we're pretty close! Some of the testing code can be cleaned up, but I could also use some help verifying that we're correctly matching the behavior of GCC as well. Comment at: test/Sema/dllexport-2.cpp:11 +// expected-warning@+3 {{__decl

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added inline comments. Comment at: lib/Sema/SemaDecl.cpp:5975-5977 +if ((!ND.isExternallyVisible() && + (!isAnonymousNS || !(VD && VD->hasInit( || + (VD && VD->isStaticLocal())) { aaron.ballman wrote: > This used to unconditionally warn

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 189843. zahiraam marked 10 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: lib/Sema/SemaDecl.cpp test/CodeGen/dllexport-1.c test/Sema/dllexport-1.cpp test/Sema/dllexpor

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:5970 auto *VD = dyn_cast(&ND); -if (!ND.isExternallyVisible() || (VD && VD->isStaticLocal())) { +const NamespaceDecl *NS = nullptr; +bool isAnonymousNS = false; This can be lo

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 189659. zahiraam marked 3 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: lib/Sema/SemaDecl.cpp test/CodeGen/dllexport-1.c test/Sema/dllexport-1.cpp test/Sema/dllexport

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:5971 auto *VD = dyn_cast(&ND); -if (!ND.isExternallyVisible() || (VD && VD->isStaticLocal())) { +NamespaceDecl *NS = NULL; +if (VD) s/NULL/nullptr Also, I think this should b

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 188047. zahiraam marked 2 inline comments as done. Herald added subscribers: jdoerfert, jfb, mgrang, srhines. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: lib/Sema/SemaDecl.cpp mypatch.patch t

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added a comment. Let's see if I have included every thing mentioned. Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://l

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11370 +// In Microsoft C++ mode, a const variable defined in namespace scope has +// external linkage by default if the variable is declared with zahiraam wrote: > aaron.ballman wrot

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam marked an inline comment as done. zahiraam added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11370 +// In Microsoft C++ mode, a const variable defined in namespace scope has +// external linkage by default if the variable is declared with ---

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11370 +// In Microsoft C++ mode, a const variable defined in namespace scope has +// external linkage by default if the variable is declared with thakis wrote: > Even in unnamed name

[PATCH] D45978: dllexport const variables must have external linkage.

thakis added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11370 +// In Microsoft C++ mode, a const variable defined in namespace scope has +// external linkage by default if the variable is declared with Even in unnamed namespaces? CHANGES SINCE

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 ___ cfe-commits mailing lis

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added a comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 186641. zahiraam marked 5 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: lib/Sema/SemaDecl.cpp test/CodeGen/dllexport-1.c test/Sema/dllexport-1.cpp test/Sema/dllexport

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. Oops, sorry, I think I added some comments to a previous revision of the review. To recap: 1. Remove the CHECK line from test/Sema/dllexport-1.cpp 2. Remove the second RUN line from test/Sema/dllexport-1.cpp and test/Sema/dllexport-2.cpp 3. Possibly remove the tri

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: test/Sema/dllexport-1.c:1 +// RUN: %clang_cc1 -triple i686-win32 -emit-llvm -fms-extensions -std=c99 < %s| FileCheck %s +// RUN: %clang_cc1 -triple x86_64-win32 -emit-llvm -fms-extensions -std=c11 < %s | FileCheck %s

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 186460. zahiraam marked 3 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: lib/Sema/SemaDecl.cpp test/CodeGen/dllexport.c test/Sema/dllexport-1.cpp test/Sema/dllexport-2

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added a subscriber: z. zahiraam added inline comments. Comment at: test/Sema/dllexport-1.c:8 + +// CHECK: @y = common dso_local dllexport global i32 0, align 4 + aaron.ballman wrote: > Are x and z also exported as expected? Only x and y are exported. *

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added inline comments. Comment at: test/Sema/dllexport-1.c:1-4 +// RUN: %clang_cc1 -triple i686-win32 -emit-llvm -fms-extensions -std=c99 < %s| FileCheck %s +// RUN: %clang_cc1 -triple x86_64-win32 -emit-llvm -fms-extensions -std=c11 < %s | FileCheck %s +// RUN: %

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 186275. Herald added a subscriber: mstorsjo. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: lib/Sema/SemaDecl.cpp test/Sema/dllexport-1.c test/Sema/dllexport-1.cpp test/Sema/dllexport-2.cpp

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added a comment. In D45978#1392855 , @aaron.ballman wrote: > It looks like the patch got mucked up somehow, I only see three testing files > in the patch now? Oops! Sorry about that. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D4597

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. It looks like the patch got mucked up somehow, I only see three testing files in the patch now? Comment at: test/Sema/dllexport.c:168 + +// CHECK: @y = common dso_local dllexport global i32 0, align 4 + Nothing runs FileCheck in

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 185920. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: test/Sema/dllexport-1.cpp test/Sema/dllexport-2.cpp test/Sema/dllexport.c Index: test/Sema/dllexport.c ==

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added a comment. In D45978#1387385 , @aaron.ballman wrote: > Can you add tests for C mode as well, as it seems the behavior differs there. Aaron, Let me know if that is enough. Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. Can you add tests for C mode as well, as it seems the behavior differs there. Given: __declspec(dllexport) int const x = 3; __declspec(dllexport) const int y; extern int const z = 4; int main() { int a = x + y + z; return a; } I get: C:\Us

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added a comment. > That seems like a reasonable place to try, to me. Ok. Let's see if this does the trick. Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 ___ cfe-commits mailing

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam updated this revision to Diff 185541. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 Files: lib/Sema/SemaDecl.cpp test/Sema/dllexport-1.cpp test/Sema/dllexport-2.cpp Index: test/Sema/dllexport-2.cpp ==

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. In D45978#1382292 , @zahiraam wrote: > In D45978#1379901 , @rnk wrote: > > > I'm still not sure this is the best place to make this change, but the > > functionality is important. The

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam added a comment. In D45978#1379901 , @rnk wrote: > I'm still not sure this is the best place to make this change, but the > functionality is important. There are still unaddressed comments (no need to > check MSVCCompatibility, formatting), and

[PATCH] D45978: dllexport const variables must have external linkage.

rnk added a comment. I'm still not sure this is the best place to make this change, but the functionality is important. There are still unaddressed comments (no need to check MSVCCompatibility, formatting), and I think once those are fixed we can land this. Comment at: test/

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a reviewer: majnemer. aaron.ballman added a subscriber: majnemer. aaron.ballman added inline comments. Comment at: test/Sema/dllexport.c:70 +// const variables +__declspec(dllexport) int const x = 3; zahiraam wrote: > aaron.ballman wrote: >

[PATCH] D45978: dllexport const variables must have external linkage.

zahiraam marked an inline comment as done. zahiraam added a comment. Aaron, Please advise. Thanks. Comment at: test/Sema/dllexport.c:70 +// const variables +__declspec(dllexport) int const x = 3; aaron.ballman wrote: > Can you think of any redeclaration sce

[PATCH] D45978: dllexport const variables must have external linkage.

rnk added a comment. I still feel like there has to be a more uniform way to handle this. Basically anything with __declspec(dllexport) on it is effectively upgraded to external linkage. Comment at: lib/Sema/SemaDeclAttr.cpp:6491 + if (auto *VD = dyn_cast(D)) { +if (getL