[PATCH] D152998: [TableGen] Support named arguments

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM w/one required comment



================
Comment at: llvm/lib/TableGen/TGParser.cpp:120
   NewName = BinOpInit::getStrConcat(NewName, Name);
-  if (CurMultiClass && Scoper != "::") {
-    Init *Prefix = BinOpInit::getStrConcat(CurMultiClass->Rec.getNameInit(),
----------------
wangpc wrote:
> reames wrote:
> > wangpc wrote:
> > > @nhaehnle
> > > It seems that we can't define nested `class` in `multiclass`, so why do we need this?
> > > Do you remember the context in D47431?
> > The removal of this code needs to be resolved.  I don't understand the context well enough to address this point.
> The removed code assumed that we can define classes inside a multiclass, so the name of outer multiclass is concatenated to the qualified name. But for current TableGen grammar, we can't define classes in multiclass, so it is unnecessary.
> 
> The reason why we need to remove it in this patch is:
> 1. We can define records, invoke class-based subroutines inside a multiclass.
> 2. When parsing the name of named arguments in `ParseTemplateArgValueList`, we need to construct the qualified name to find if there is an argument with that name.
> 3. `CurMultiClass` may not be null, so the qualified name woule be like `MultiClass::Class:argname`, which doesn't exist.
> 4. Then we fail to find the argument which exists actually.
> 
> 
Please separate this diff, and commit it separately.  Your explanation is sufficient, and you don't need separate review.  You should take the time to construct a commit comment which explains which this is fully NFC (without this patch).  


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152998/new/

https://reviews.llvm.org/D152998