OikawaKirie requested review of this revision.
OikawaKirie added a comment.
@aaron.ballman
My only concern is the recursive call to this function on line 1359. If you
think it is also OK for the recursive call on line 1359, I will update the
patch as you mentioned above.
BTW, how can I test and debug this tblgen backend?
================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1359
for (const auto &Base : llvm::reverse(Bases)) {
if ((Ptr = createArgument(Arg, Attr, Base.first)))
break;
----------------
The recursive call is here.
The for loop here seems to allow a failed attempt on its bases. Although the
top-level callers expect the return value of this function is always non-null,
what about the call here?
================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1364
+ assert(Ptr && "Cannot create argument.");
+
----------------
aaron.ballman wrote:
> 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.
>
However, there is a recursive call to this function on line 1359, and it seems
that `nullptr` is allowed to be returned for the call. I am just worrying about
whether the assertion here will lead to a crash in the recursive calls.
IMO, a better way is to wrap this function and assert the return value of this
function in the wrapper function.
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
