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

Reply via email to