[PATCH] D13925: Implement __attribute__((internal_linkage))

[Evgeniy Stepanov via cfe-commits]

eugenis added inline comments.

================
Comment at: include/clang/Basic/Attr.td:2125
@@ +2124,3 @@
+
+def InternalLinkage : InheritableAttr {
+  let Spellings = [GNU<"internal_linkage">, CXX11<"clang", "internal_linkage">];
----------------
aaron.ballman wrote:
> rsmith wrote:
> > `InheritableAttr` doesn't seem right for the case when this is applied to a namespace.
> Long and unusual is not a problem; that's why you can manually specify the diagnostic enum. This should be using Subjects unless it's reallllly onerous (or impossible).
This attribute no longer applies to namespaces.

================
Comment at: include/clang/Basic/AttrDocs.td:1619
@@ +1618,3 @@
+  let Content = [{
+The ``internal_linkage`` attribute changes the linkage type of the declaration to internal.
+  }];
----------------
aaron.ballman wrote:
> Some examples would be nice, as well as an explanation as to why someone might want to do this.
Extended the explanation.

================
Comment at: lib/AST/Decl.cpp:1358-1362
@@ -1346,3 +1357,7 @@
     }
-    assert(!Old || Old->getCachedLinkage() == D->getCachedLinkage());
+    // Linkages may also differ if one of the declarations has
+    // InternalLinkageAttr.
+    assert(!Old || Old->getCachedLinkage() == D->getCachedLinkage() ||
+           (Old->hasAttr<InternalLinkageAttr>() !=
+            D->hasAttr<InternalLinkageAttr>()));
 #endif
----------------
rsmith wrote:
> We shouldn't allow the linkage to be changed after the first declaration. What cases cause problems here?
Isn't it the same case as in the comment above with static following an extern in microsoft extensions?


================
Comment at: lib/Sema/Sema.cpp:539
@@ -538,3 +538,3 @@
 
-    if (!ND->isExternallyVisible()) {
+    if (!ND->isExternallyVisible() || ND->hasAttr<InternalLinkageAttr>()) {
       S.Diag(ND->getLocation(), diag::warn_undefined_internal)
----------------
rsmith wrote:
> Hmm, what should `isExternallyVisible` return for a declaration with this attribute? Presumably it should be `false`...
Fixed by adding an attribute check to getLVForDecl (in addition to computeLVForDecl).


Repository:
  rL LLVM

http://reviews.llvm.org/D13925