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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman requested changes to this revision.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.
This revision now requires changes to proceed.

I would like to hold off on adding the namespace attribute. There were persuasive reasons to not have attributes on namespaces that was discussed in EWG in Kona, and this is a feature we could add on later if there's sufficient design consensus.


================
Comment at: include/clang/Basic/Attr.td:2125
@@ +2124,3 @@
+
+def InternalLinkage : InheritableAttr {
+  let Spellings = [GNU<"internal_linkage">, CXX11<"clang", "internal_linkage">];
----------------
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).

================
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.
+  }];
----------------
Some examples would be nice, as well as an explanation as to why someone might want to do this.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:3306
@@ +3305,3 @@
+
+  if (InternalLinkageAttr *Internal = D->getAttr<InternalLinkageAttr>()) {
+    Diag(Range.getBegin(), diag::err_attributes_are_not_compatible)
----------------
Please use checkAttrMutualExclusion instead (and augment it with the note_conflicting_attribute).

================
Comment at: lib/Sema/SemaDeclAttr.cpp:3320
@@ +3319,3 @@
+
+  if (CommonAttr *Common = D->getAttr<CommonAttr>()) {
+    Diag(Range.getBegin(), diag::err_attributes_are_not_compatible)
----------------
Please use checkAttrMutualExclusion instead.

================
Comment at: test/SemaCXX/internal_linkage.cpp:6
@@ +5,3 @@
+public:
+  int x __attribute__((internal_linkage)); // expected-warning{{'internal_linkage' attribute ignored}}
+  static int y __attribute__((internal_linkage));
----------------
This does not seem like a useful diagnostic as it doesn't tell the user *why* it was ignored.

================
Comment at: test/SemaCXX/internal_linkage.cpp:25
@@ +24,2 @@
+void A::f1() {
+}
----------------
Missing tests for internal_linkage accepting an argument (which should diagnose).
Missing tests for what happens with forward declarations and redeclarations that do not have matching attribute specifiers.
Missing tests for using the C++ spelling of the attribute.


Repository:
  rL LLVM

http://reviews.llvm.org/D13925