aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM with a few minor changes to be made. ================ Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1364 + assert(Ptr && "Cannot create argument."); + ---------------- OikawaKirie wrote: > Just as what has been suggested by @dexonsmith in D91844, I add the assertion > here. However, as a recursive function it is (line 1359), I still concern > whether the assertion here will lead to a crash. > > What do you think? The intent with this function is that it never returns `nullptr` unless the programmer messed up their call of the function (passed in the wrong kind of `Record` kind of problem). All of the callers assume this returns non-null, so I think it's reasonable to assert that the value is non-null by this point. ================ Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1364 + assert(Ptr && "Cannot create argument."); + ---------------- aaron.ballman wrote: > OikawaKirie wrote: > > Just as what has been suggested by @dexonsmith in D91844, I add the > > assertion here. However, as a recursive function it is (line 1359), I still > > concern whether the assertion here will lead to a crash. > > > > What do you think? > The intent with this function is that it never returns `nullptr` unless the > programmer messed up their call of the function (passed in the wrong kind of > `Record` kind of problem). All of the callers assume this returns non-null, > so I think it's reasonable to assert that the value is non-null by this point. ================ Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1366-1370 if (Ptr && Arg.getValueAsBit("Optional")) Ptr->setOptional(true); if (Ptr && Arg.getValueAsBit("Fake")) Ptr->setFake(true); ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97251/new/ https://reviews.llvm.org/D97251 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits