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/Sema/dllexport.c:70
+// const variables
+__declspec(dllexport) int const x = 3;
----------------
aaron.ballman wrote:
> zahiraam wrote:
> > aaron.ballman wrote:
> > > Can you think of any redeclaration scenarios we should also test? e.g.,
> > > weird cases like this:
> > > ```
> > > extern int const x;
> > > __declspec(dllexport) int const x = 3;
> > > ```
> > > which brings up an interesting question -- does MSVC truly treat it as
> > > though it were extern, or just only-kinda-sorta treat it like it was
> > > extern? e.g., can you do this:
> > > ```
> > > __declspec(dllexport) const int j;
> > > ```
> > > Regardless, having some codegen tests demonstrating that the variable has
> > > the correct semantics that we're emulating would be nice.
> > With MSVC:
> >
> > ksh-3.2$ cat t2.cpp
> > __declspec(dllexport) const int j;
> > ksh-3.2$
> >
> > ksh-3.2$ cl -c t2.cpp
> > Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
> > Copyright (C) Microsoft Corporation. All rights reserved.
> >
> > t2.cpp
> > t2.cpp(1): error C2734: 'j': 'const' object must be initialized if not
> > 'extern'
> >
> > ksh-3.2$ cat t3.cpp
> > __declspec(dllexport) int const x = 3;
> > int main ()
> > {
> > int y = x + 10;
> >
> > return y;
> > }
> >
> > ksh-3.2$ cl -c t3.cpp
> > Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
> > Copyright (C) Microsoft Corporation. All rights reserved.
> >
> > t3.cpp
> > ksh-3.2$ dumpbin /symbols t3.obj | grep External
> > **008 00000000 SECT3 notype () External | main**
> > ksh-3.2$
> > ksh-3.2$ cat t4.cpp
> > extern int const x = 3;
> > int main ()
> > {
> > int y = x + 10;
> >
> > return y;
> > }
> >
> > ksh-3.2$ cl -c t4.cpp
> > Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24210 for x64
> > Copyright (C) Microsoft Corporation. All rights reserved.
> >
> > t4.cpp
> > ksh-3.2$ dumpbin /symbols t4.obj | grep External
> > **008 00000000 SECT3 notype External | ?x@@3HB (int const x)
> > 00B 00000000 SECT4 notype () External | main**
> > ksh-3.2$
> >
> > So we see that with dllexport, the ony symbol marked with "External" is
> > main. With "extern" both main and x are marked as External.
> >
> > With clang without the patch:
> > ksh-3.2$ clang -c t2.cpp
> > t2.cpp:1:33: error: default initialization of an object of const type
> > 'const int'
> > __declspec(dllexport) const int j;
> > ^
> > = 0
> > t2.cpp:1:33: error: 'j' must have external linkage when declared 'dllexport'
> > 2 errors generated.
> > ksh-3.2$
> > ksh-3.2$ clang -c t3.cpp
> > t3.cpp:1:33: error: 'x' must have external linkage when declared 'dllexport'
> > __declspec(dllexport) int const x = 3;
> > ^
> > 1 error generated.
> > ksh-3.2$ clang -c t4.cpp
> > ksh-3.2$ dumpbin /symbols t4.o | grep External
> > **00F 00000000 SECT1 notype () External | main
> > 010 00000000 SECT5 notype External | ?x@@3HB (int const x)**
> > ksh-3.2$
> >
> > Clang with patch above at the right place (I am thinking in
> > Sema::AddInitializerToDecl):
> >
> > ksh-3.2$ clang -c t3.cpp
> > ksh-3.2$ dumpbin /symbols t3.o | grep External
> > **00E 00000000 SECT1 notype () External | main
> > 00F 00000000 SECT5 notype External | ?x@@3HB (int const x)**
> > ksh-3.2$
> > ksh-3.2$ clang -c t4.cpp
> > ksh-3.2$ dumpbin /symbols t4.o | grep External
> > 00C 00000000 SECT1 notype () External | main
> > 00D 00000000 SECT5 notype External | ?x@@3HB (int const x)
> > ksh-3.2$
> >
> > Both MSVC and clang have the same behavior with t2.cpp.
> > With the patch clang will have the same beahvior than MSVC when extern and
> > dllexport are used. That's not quite what MSVC's behavior is!
> > What are your thoughts?
> > Thanks.
> >
> >
> Neat, so MSVC treats it as sort-of-extern-but-sort-of-not. I can't tell
> whether this is intentional and we want to be compatible, or whether this is
> a bug in MSVC (and if so, is it one uses are relying on and we need to
> emulate). I'm not certain what the best answer is here. Perhaps @rnk or
> @majnemer have opinions?
Yes, we should be compatible in this area, it's an extension that we implement,
not invalid C++.
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
