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

[Aaron Ballman via Phabricator via cfe-commits]

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