[Sema] raise nullptr check to cover additional uses

[nickdesaulniers]

Summary

This was flagged in https://www.viva64.com/en/b/0629/ under "Snippet No.
14" (see under #13). It looks like PVS studio flags nullptr checks where
the ptr is used inbetween creation and checking against nullptr.

[nickdesaulniers]

Note that this changes when the destructor for CXXThisScopeRAII runs. It's not clear to me why ThisScope is constructed at all.

[nickdesaulniers]

A similar case exists and was flagged in https://www.viva64.com/en/b/0629/ under "Snippet No. 16" (see under #13), clang/lib/Sema/SemaTemplateInstantiate.cpp Sema::InstantiateClass(). I'll wait for feedback on this patch, then either roll up the additional fix into this or create it as a separate patch (or change both, abandon, etc).

[rsmith]

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

557–558

The purpose of creating this is to change the type of this in the scope of the instantiation below, so this change is incorrect. The right fix would be to change the initializer of ThisContext to handle ND being null. (I suspect that actually can't happen at the moment because we happen to only attach late-parsed attributes to NamedDecls, but I don't think there's any fundamental reason why that should be the case so it makes some sense to retain the check here.)

[nickdesaulniers]

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

557–558

The purpose of creating this is to change the type of this in the scope of the instantiation below, so this change is incorrect.

In this case I agree. The } added on L559 should be extended to L564.

so it makes some sense to retain the check here

I assume PVS studio is complaining about dereferencing ND on L556, then doing what looks like a nullptr check on L558 (reminiscent of -fno-delete-null-pointer-checks). I assume dyn_cast on L554 can fail at runtime and return nullptr? Should that happen, how should we proceed?

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

if (TmplAttr->isLateParsed() && LateAttrs) {
       // Late parsed attributes must be instantiated and attached after the
       // enclosing class has been instantiated.  See Sema::InstantiateClass.
       LocalInstantiationScope *Saved = nullptr;
       if (CurrentInstantiationScope)
         Saved = CurrentInstantiationScope->cloneScopes(OuterMostScope);
       LateAttrs->push_back(LateInstantiatedAttribute(TmplAttr, Saved, New));
     } else {
       // Allow 'this' within late-parsed attributes.
-      NamedDecl *ND = dyn_cast<NamedDecl>(New);
+      if (NamedDecl *ND = dyn_cast<NamedDecl>(New)) {
         CXXRecordDecl *ThisContext =
             dyn_cast_or_null<CXXRecordDecl>(ND->getDeclContext());
         CXXThisScopeRAII ThisScope(*this, ThisContext, Qualifiers(),
-                                 ND && ND->isCXXInstanceMember());
+                                   ND->isCXXInstanceMember());
+      }
 
       Attr *NewAttr = sema::instantiateTemplateAttribute(TmplAttr, Context,
                                                          *this, TemplateArgs);
       if (NewAttr)
         New->addAttr(NewAttr);
     }
   }
 }