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

[Aaron Ballman via Phabricator via cfe-commits]

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 wrote:
> > thakis wrote:
> > > Even in unnamed namespaces?
> > That would definitely be good to test.
> It looks like not. 
> 
> ksh-3.2$ cat test4.cpp
> namespace {
> __declspec(dllexport) int const x = 3;
> }
> 
> int main ()
> {
>   int y = x + 10;
> 
>   return y;
> }
> 
> ksh-3.2$ cl -c test4.cpp
> Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
> Copyright (C) Microsoft Corporation.  All rights reserved.
> 
> test4.cpp
> ksh-3.2$ dumpbin /symbols test4.obj | grep External
> **008 00000000 SECT3  notype ()    External     | main**
> ksh-3.2$
> 
> ksh-3.2$ clang -c test4.cpp
> test4.cpp:2:33: error: '(anonymous namespace)::x' must have external linkage when declared 'dllexport'
> __declspec(dllexport) int const x = 3;
>                                 ^
> 1 error generated.
> ksh-3.2$
> 
> So the diag shouldn't go off when the variable is in an anonymous namespace? Do you agree?
> 
> So the diag shouldn't go off when the variable is in an anonymous namespace? Do you agree?

Correct.


================
Comment at: test/Sema/dllexport-2.cpp:5
+
+__declspec(dllexport) int const j; // expected-error {{default initialization of an object of const type 'const int'}} // expected-error {{'j' must have external linkage when declared 'dllexport'}}
----------------
Another test I'd like to see is using a typedef to apply the const qualification. e.g.,
```
typedef const int CInt;
__declspec(dllexport) CInt j2;
__declspec(dllexport) CInt j3 = 3;
```
We should match the MSVC behavior here as well (I wonder if your use of `isLocalConstQualified()` should actually be `isConstQualified()` instead).


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

https://reviews.llvm.org/D45978