aaron.ballman added a comment.
In D97251#2583843 <https://reviews.llvm.org/D97251#2583843>, @OikawaKirie wrote:
> @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?
There really is no way to test these changes, but you can debug the generator
by running `clang-tblgen -gen-whatever-you-want -I path\to\clang\include
path\to\clang\include\clang\Basic\Attr.td` (the available generators are listed
in clang\utils\TableGen\TableGen.cpp).
================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1359
for (const auto &Base : llvm::reverse(Bases)) {
if ((Ptr = createArgument(Arg, Attr, Base.first)))
break;
----------------
OikawaKirie wrote:
> 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?
I believe this call is fine as-is.
================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1364
+ assert(Ptr && "Cannot create argument.");
+
----------------
OikawaKirie wrote:
> 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.
I don't think we need to go to those lengths -- if the recursive call fails,
there was a Clang programmer mistake (they wrote the wrong thing in Attr.td or
they forgot to write the right things in ClangAttrEmitter.cpp).
Put another way: we could use `PrintFatalError(Arg.getLoc(), "Failed to create
the argument")` instead of `assert` if we wanted.
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
