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@+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@+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@+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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
