[PATCH] D97251: [llvm] Add assertions for the smart pointers with the possibility to be null in ClangAttrEmitter

[Ella Ma via Phabricator via cfe-commits]

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