asavonic marked 5 inline comments as done. asavonic added a comment. In https://reviews.llvm.org/D49725#1173321, @yaxunl wrote:
> This patch also adds check for array of structs. Can you include this in > title or split to a separate patch? I'm sorry, this change with arrays should actually go into https://reviews.llvm.org/D49723. Moved it there. ================ Comment at: lib/Sema/SemaDecl.cpp:8065 + std::find(Names.begin(), Names.end(), DesugaredTy.getAsString()); + if (Names.end() != Match) + return true; ---------------- yaxunl wrote: > Can we record the real size_t/intptr_t/etc typedef and later on emit a note > for it? It helps user to locate the culprit typedef in case of multiple > typedefs. I changed the patch to emit a note for all typedefs involved in InvalidKernelParam. This way it works not only for size_t types, but also for other invalid types which were typedef'ed. ================ Comment at: lib/Sema/SemaDecl.cpp:8186 - const RecordDecl *PD = PT->castAs<RecordType>()->getDecl(); - VisitStack.push_back(PD); + // At this point we already handled everything except of a RecordType or + // an ArrayType[RecordType]. ---------------- Anastasia wrote: > Anastasia wrote: > > I am a bit confused about this comment, `do you mean a PointerType to a > > RecordType or an ArrayType of a RecordType`? > Also is there any test case covering this change? ArrayType of a RecordType. Fixed. ================ Comment at: lib/Sema/SemaDecl.cpp:8186 - const RecordDecl *PD = PT->castAs<RecordType>()->getDecl(); - VisitStack.push_back(PD); + // At this point we already handled everything except of a RecordType or + // an ArrayType[RecordType]. ---------------- asavonic wrote: > Anastasia wrote: > > Anastasia wrote: > > > I am a bit confused about this comment, `do you mean a PointerType to a > > > RecordType or an ArrayType of a RecordType`? > > Also is there any test case covering this change? > ArrayType of a RecordType. Fixed. I added 2 tests for this change in https://reviews.llvm.org/D49723. ================ Comment at: lib/Sema/SemaDecl.cpp:8189 + const RecordType *RecTy = + PT->getPointeeOrArrayElementType()->getAs<RecordType>(); + const RecordDecl *OrigRecDecl = RecTy->getDecl(); ---------------- Anastasia wrote: > yaxunl wrote: > > Can we have a test for this change? e.g. an array of structs > I am wondering if `PT->getPointeeOrArrayElementType()` is `nullptr`? Do we > need to add an extra check? Added in https://reviews.llvm.org/D49723. ================ Comment at: lib/Sema/SemaDecl.cpp:8189 + const RecordType *RecTy = + PT->getPointeeOrArrayElementType()->getAs<RecordType>(); + const RecordDecl *OrigRecDecl = RecTy->getDecl(); ---------------- asavonic wrote: > Anastasia wrote: > > yaxunl wrote: > > > Can we have a test for this change? e.g. an array of structs > > I am wondering if `PT->getPointeeOrArrayElementType()` is `nullptr`? Do we > > need to add an extra check? > Added in https://reviews.llvm.org/D49723. It should not return null, unless the PT is null: https://clang.llvm.org/doxygen/Type_8h_source.html#l06487 Repository: rC Clang https://reviews.llvm.org/D49725 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
