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

[Aaron Ballman via Phabricator via cfe-commits]

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
----------------
aaron.ballman wrote:
> This test should live in CodeGen not Sema.
Do we need this many RUN lines? Also, why the different language modes? Also, do you need to use `<` in the command?


================
Comment at: test/Sema/dllexport-1.c:8
+
+// CHECK: @y = common dso_local dllexport global i32 0, align 4
+
----------------
zahiraam wrote:
> aaron.ballman wrote:
> > Are x and z also exported as expected?
> Only x and y are exported.
> 
> 
> **@x = dso_local dllexport constant i32 3, align 4**
> @z = dso_local constant i32 4, align 4
> **@y = common dso_local dllexport global i32 0, align 4**
> 
> But then if I take this simple case:
> extern int const z = 4;
> 
> int main() {
>   int a = z + 2;
>   return a;
> }
> ksh-3.2$ clang -c test3.c
> test3.c:1:18: warning: 'extern' variable has an initializer [-Wextern-initializer]
> extern int const z = 4;
>                  ^
> 1 warning generated.
> ksh-3.2$ dumpbin /symbols test3.o | grep External
> 00F 00000000 SECT1  notype ()    External     | main
> **010 00000000 SECT5  notype       External     | z**
> ksh-3.2$
> When emitting the IR, z is declared as a local constant (not exported):
> @z = dso_local constant i32 4, align 4
> 
> 
Okay, please add the expected CHECK lines for `x` and `z` to the test. Thank you for clarifying!


================
Comment at: test/Sema/dllexport-1.cpp:2
+// RUN: %clang_cc1 -triple i686-win32 -fsyntax-only -fms-extensions -verify %s
+// RUN: %clang_cc1 -triple x86_64-win32 -fsyntax-only -fms-extensions -verify -std=c++11 %s
+
----------------
I don't think we need this RUN line. I don't know if we need the triple in the previous RUN line either.


================
Comment at: test/Sema/dllexport-1.cpp:4
+
+// CHECK: @"?x@@3HB" = dso_local dllexport constant i32 3, align 4
+
----------------
aaron.ballman wrote:
> This test has no FileCheck line.
> 
> The goal here is for the tests in Sema to test the semantic behavior (that warnings are issued when expected and not issued when not expected) and to add tests in CodeGen to test the generated code (are things properly exported, are things we don't expect to be exported not exported, etc). I think you should split your tests accordingly rather than try to verify too much at once.
This CHECK line still has no impact; I think it should be removed from the test.


================
Comment at: test/Sema/dllexport-2.cpp:2
+// RUN: %clang_cc1 -triple i686-win32 -fsyntax-only -fms-extensions -verify %s
+// RUN: %clang_cc1 -triple x86_64-win32 -fsyntax-only -fms-extensions -verify -std=c++11 %s
+
----------------
I don't think we need this RUN line. I don't know if we need the triple in the previous RUN line either.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D45978/new/

https://reviews.llvm.org/D45978