smeenai created this revision.
smeenai added reviewers: compnerd, hans.
smeenai added a subscriber: cfe-commits.
On MSVC, if an implicit instantiation already exists and an explicit
instantiation definition with a DLL attribute is created, the DLL
attribute still takes effect. Make clang match thi
compnerd added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7669
+// Fix a TSK_ExplicitInstantiationDeclaration or a
TSK_ImplicitInstantiation
+// followed by a TSK_ExplicitInstantiationDefinition
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
---
smeenai added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7669
+// Fix a TSK_ExplicitInstantiationDeclaration or a
TSK_ImplicitInstantiation
+// followed by a TSK_ExplicitInstantiationDefinition
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
hans added a comment.
> On MSVC, if an implicit instantiation already exists and an explicit
> instantiation definition with a DLL attribute is created, the DLL
> attribute still takes effect. Make clang match this behavior.
This is scary territory, and behaviour I think might be hard for us to
smeenai added a comment.
In https://reviews.llvm.org/D26657#596759, @hans wrote:
> > On MSVC, if an implicit instantiation already exists and an explicit
> > instantiation definition with a DLL attribute is created, the DLL
> > attribute still takes effect. Make clang match this behavior.
>
> T
hans added a comment.
In https://reviews.llvm.org/D26657#596897, @smeenai wrote:
> In https://reviews.llvm.org/D26657#596759, @hans wrote:
>
> > > On MSVC, if an implicit instantiation already exists and an explicit
> > > instantiation definition with a DLL attribute is created, the DLL
> > > a
smeenai added a comment.
In https://reviews.llvm.org/D26657#597523, @hans wrote:
> In https://reviews.llvm.org/D26657#596897, @smeenai wrote:
>
> > In https://reviews.llvm.org/D26657#596759, @hans wrote:
> >
> > > > On MSVC, if an implicit instantiation already exists and an explicit
> > > > ins
hans added a comment.
In https://reviews.llvm.org/D26657#597859, @smeenai wrote:
> In https://reviews.llvm.org/D26657#597523, @hans wrote:
>
> > In https://reviews.llvm.org/D26657#596897, @smeenai wrote:
> >
> > > In https://reviews.llvm.org/D26657#596759, @hans wrote:
> > >
> > > > > On MSVC, if
rnk added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7670-7673
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
(TSK == TSK_ExplicitInstantiationDefinition ||
DLLImportExplicitInstantiat
smeenai added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7670-7673
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
(TSK == TSK_ExplicitInstantiationDefinition ||
DLLImportExplicitInstan
smeenai updated this revision to Diff 78322.
smeenai added a comment.
Limiting to dllexport after discussion with @hans
https://reviews.llvm.org/D26657
Files:
lib/Sema/SemaTemplate.cpp
test/CodeGenCXX/dllexport.cpp
test/CodeGenCXX/windows-itanium-dllexport.cpp
Index: test/CodeGenCXX/win
majnemer added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7683-7685
+// or implicit instantiation. MinGW doesn't allow this. In the implicit
+// instantiation case, we limit clang to only adding dllexport; see the
+// discussion in https://revi
smeenai updated this revision to Diff 78324.
smeenai added a comment.
Moving explanation to comment instead of referencing Phabricator
https://reviews.llvm.org/D26657
Files:
lib/Sema/SemaTemplate.cpp
test/CodeGenCXX/dllexport.cpp
test/CodeGenCXX/windows-itanium-dllexport.cpp
Index: test
hans added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7670-7673
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
(TSK == TSK_ExplicitInstantiationDefinition ||
DLLImportExplicitInstantia
smeenai added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7670-7673
+if ((Old_TSK == TSK_ExplicitInstantiationDeclaration ||
+ Old_TSK == TSK_ImplicitInstantiation) &&
(TSK == TSK_ExplicitInstantiationDefinition ||
DLLImportExplicitInstan
smeenai updated this revision to Diff 78620.
smeenai updated the summary for this revision.
smeenai added a comment.
Addressing comments
https://reviews.llvm.org/D26657
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaTemplate.cpp
test/CodeGenCXX/dllexport.cpp
test/CodeGenCXX/windows-itani
smeenai updated this revision to Diff 78621.
smeenai added a comment.
Typo in comment
https://reviews.llvm.org/D26657
Files:
include/clang/Sema/Sema.h
lib/Sema/SemaTemplate.cpp
test/CodeGenCXX/dllexport.cpp
test/CodeGenCXX/windows-itanium-dllexport.cpp
Index: test/CodeGenCXX/windows-it
smeenai added a comment.
General coding style question. Over here, I'm creating a local helper function.
However, that helper needs to access member functions of Sema, which is why I
made it a private member function right now, which also unfortunately makes the
change somewhat non-local (since
smeenai added a comment.
Ping.
https://reviews.llvm.org/D26657
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hans added a comment.
Apologies for the delay. I was out last week.
In https://reviews.llvm.org/D26657#602083, @smeenai wrote:
> General coding style question. Over here, I'm creating a local helper
> function. However, that helper needs to access member functions of Sema,
> which is why I mad
hans added inline comments.
Comment at: include/clang/Sema/Sema.h:7495
+ /// \brief Make an existing DLL attribute on a class take effect.
+ void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+InheritableAttr *Attr);
hans wrote:
> I'd s
smeenai added inline comments.
Comment at: include/clang/Sema/Sema.h:7494
+ /// \brief Make an existing DLL attribute on a class take effect.
+ void ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
hans wrote:
> Nit: I think `///` implies `\brief`, so we do
hans added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7439
+void Sema::ActOnDLLAttr(ClassTemplateSpecializationDecl *Def,
+InheritableAttr *Attr) {
+ // We reject explicit instantiations in class scope, so there should
smeenai
smeenai updated this revision to Diff 80193.
smeenai added a comment.
Addressing comments
https://reviews.llvm.org/D26657
Files:
lib/Sema/SemaTemplate.cpp
test/CodeGenCXX/dllexport.cpp
test/CodeGenCXX/windows-itanium-dllexport.cpp
Index: test/CodeGenCXX/windows-itanium-dllexport.cpp
smeenai added inline comments.
Comment at: lib/Sema/SemaTemplate.cpp:7710
+(Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+ Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment())) {
+ // In the MS ABI, an explicit instantiation definition c
hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.
lgtm
Comment at: lib/Sema/SemaTemplate.cpp:7710
+(Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+ Context.getTargetInfo().getTriple().isWindowsItaniumEnvir
This revision was automatically updated to reflect the committed changes.
Closed by commit rL288682: [Sema] Respect DLL attributes more faithfully
(authored by smeenai).
Changed prior to commit:
https://reviews.llvm.org/D26657?vs=80193&id=80289#toc
Repository:
rL LLVM
https://reviews.llvm.o
27 matches
Mail list logo
