[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Keno Fischer via cfe-commits]

loladiro added inline comments.

================
Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+  let Spellings = [GNU<"unique_instantiation">];
+  let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+  let Documentation = [UniqueInstantiationDocs];
----------------
majnemer wrote:
> What about variable templates?
I hadn't tried before, but looking at this, I'd say they're broken in clang anyway and I'm gonna say fixing that is outside the scope of this revision:
```
$ cat test.cpp
template <typename T> T n;
extern template int n<int>;
template int n<int>;

int main() {
    return n<int>;
}
$ ./usr/bin/clang++ -std=c++14 -c test.cpp
$ nm test.o
0000000000000000 T main
U _Z1nIiE
$ nm test.o.gcc6
[snip]
00000000060098c u _Z1nIiE
```

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
@@ -2450,1 +2455,3 @@
+def err_unique_instantiation_not_previous : Error<
+  "The unique_instantiation must be specified on all declarations and definitions of a particular explicit template instantiation.">;
 
----------------
aaron.ballman wrote:
> Should say: the 'unique_instantiation' attribute.
> 
> Also, I'm not keen on "a particular" as it's very non-specific.
> 
> I wonder if we need err_unique_instantiation_no_declaration and err_unique_instantiation_not_previous to be separate diagnostics? Aren't they both effectively saying, 
> 
> "a 'unique_instantiation' attribute must be specified for all declarations and definitions of the explicit template instantiation"
They are checking for two different conditions in the spec. One requires that all explicit template instantiations with this attribute have a declaration, the other that every declaration/definition has the attribute.

================
Comment at: test/CodeGenCXX/unique-instantiation.cpp:1
@@ +1,2 @@
+// RUN: %clang -std=c++11 -emit-llvm -O0 -c -S -o - %s | FileCheck %s
+
----------------
majnemer wrote:
> Is -O0 needed here?
No, removing.


http://reviews.llvm.org/D13330