[PATCH] D24508: PR28752: Do not instantiate var decls which are not visible.

[Vassil Vassilev via cfe-commits]

v.g.vassilev marked 2 inline comments as done.
v.g.vassilev added a comment.

Landed in r283882.



================
Comment at: include/clang/AST/Decl.h:1222
+  void demoteThisDefinitionToDeclaration() {
+    assert (!isThisDeclarationADemotedDefinition() && "Aleady demoted!");
+    assert (isThisDeclarationADefinition() && "Not a definition!");
----------------
rsmith wrote:
> You can remove this; it's covered by the next line.
Ok. This would allow calling the method on already demoted definition. Do we want to allow that?


================
Comment at: lib/AST/Decl.cpp:2284
+      while (auto *NewVD = VD->getInstantiatedFromStaticDataMember())
+        VD = NewVD;
+      return VD->getDefinition();
----------------
rsmith wrote:
> Missing a member specialization check here.
Isn't that covered by the cases above? It seems there is no API to make such check, here. The current state looks to me consistent with `CXXRecordDecl::getTemplateInstantiationPattern` and `FunctionDecl::getTemplateInstantiationPattern`.


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:3086-3090
+      if (CurD->isThisDeclarationADemotedDefinition()) {
+        VD->demoteThisDefinitionToDeclaration();
+        break;
+      }
+      if (CurD->isThisDeclarationADefinition()) {
----------------
rsmith wrote:
> Maybe combine these two `if`s into one, since their bodies are identical?
Good point ;)


https://reviews.llvm.org/D24508