[PATCH] D62116: [Sema] raise nullptr check to cover additional uses
Nick Desaulniers via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue May 21 16:46:59 PDT 2019
nickdesaulniers added inline comments.
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:557-558
+ dyn_cast_or_null<CXXRecordDecl>(ND->getDeclContext());
+ CXXThisScopeRAII ThisScope(*this, ThisContext, Qualifiers(),
+ ND->isCXXInstanceMember());
+ }
----------------
rsmith wrote:
> 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 `NamedDecl`s, 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.)
> 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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62116/new/
https://reviews.llvm.org/D62116
More information about the cfe-commits
mailing list