[PATCH] D45978: dllexport const variables must have external linkage.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:5970
     auto *VD = dyn_cast<VarDecl>(&ND);
-    if (!ND.isExternallyVisible() || (VD && VD->isStaticLocal())) {
+    const NamespaceDecl *NS = nullptr;
+    bool isAnonymousNS = false;
----------------
This can be lowered into the below `if` statement.


================
Comment at: lib/Sema/SemaDecl.cpp:5971-5972
+    const NamespaceDecl *NS = nullptr;
+    bool isAnonymousNS = false;
+    bool isMicrosoft = S.Context.getTargetInfo().getCXXABI().isMicrosoft();
+    if (VD) {
----------------
is -> Is per our naming convention rules.


================
Comment at: lib/Sema/SemaDecl.cpp:5977
+      while (DC && NS) {
+        isAnonymousNS = isAnonymousNS || (NS && NS->getDeclName().isEmpty());
+        DC = DC->getParent();
----------------
No need to test that `NS` is nonnull here; already done as part of the while loop predicate. Also, you can call `NamespaceDecl::isAnonymousNamespace()` rather than manually try to test the declared identifier.

I think a more clear way to write this is:
```
if (VD) {
  const NamespaceDecl *NS = dyn_cast<NamespaceDecl>(VD->getDeclContext());
  while (NS && !IsAnonymousNS) {
    IsAnonymousNS = NS->isAnonymousNamespace();
    NS = dyn_cast<NamespaceDecl>(NS->getParent());
  }
  ...
}
```


================
Comment at: lib/Sema/SemaDecl.cpp:5982-5984
+    if ((ND.isExternallyVisible() && isAnonymousNS && isMicrosoft) ||
+        (!(isAnonymousNS && isMicrosoft) &&
+         (!ND.isExternallyVisible() || (VD && VD->isStaticLocal())))) {
----------------
This is a pretty complicated predicate; I'd simplify it a bit and then add some comments to explain it further. The comments from line 5967 could move down here (with modification), in fact. Untested:
```
    bool AnonNSInMicrosoftMode = IsAnonymous && IsMicrosoft;
    if ((ND.isExternallyVisible() && AnonNSInMicrosoftMode) ||
        (!AnonNSInMicrosoftMode &&
         (!ND.isExternallyVisible() || VD->isStaticLocal()))) {
      ...
    }
```


================
Comment at: test/Sema/dllexport-1.cpp:7
+#if MSVC
+// expected-no-diagnostics at +4
+#else
----------------
I am almost certain that `expected-no-diagnostics` applies to the entire file, so the @ doesn't make sense. I think you just need one of these for the entire file:
```
#ifdef MSVC
// expected-no-diagnostics
#endif
```
(Note, I switched it to use `#ifdef` as well.)


================
Comment at: test/Sema/dllexport-2.cpp:6
+
+#if MSVC
+// expected-error at +6 {{default initialization of an object of const type 'const int'}}
----------------
Please switch to `#ifdef` rather than `#if`.


================
Comment at: test/Sema/dllexport-2.cpp:28
+#if MSVC
+// expected-nodiagnostics
+#else
----------------
This isn't correct -- had the typo not been there, then the test would have failed because the file has diagnostics in MSVC mode. This should read:
```
#ifndef MSVC
// expected-warning at +2 {{__declspec attribute 'dllexport' is not supported}}
#endif
```


================
Comment at: test/SemaCXX/dllexport.cpp:73
+#ifdef MS
+// expected-nodiagnostics
+#else
----------------
Typo, but this is the wrong construct. Should be:
```
#ifndef MS
namespace {
  __declspec(dllexport) int InternalGlobal; // expected-error{{anonymous namespace)::InternalGlobal' must have external linkage when declared 'dllexport'}}
}
#endif
```


================
Comment at: test/SemaCXX/dllexport.cpp:133
+#ifdef MS
+// expected-nodiagnostics
+#else
----------------
Similar here as above.


================
Comment at: test/SemaCXX/dllimport.cpp:125
+#ifdef MS
+// expected-nodiagnostics
+#else
----------------
Here too.


================
Comment at: test/SemaCXX/dllimport.cpp:222
+#ifdef MS
+// expected-nodiagnostics
+#else
----------------
And here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D45978/new/

https://reviews.llvm.org/D45978