A new Init type ArgumentInit is added to represent arguments, the
original way that we pass arguments in positional way is represented
as PositionalArgument.
The index of argument in error message is removed.
Paths
| Differential D154066
[NFC][TableGen] Refactor the implementation of arguments ClosedPublic Authored by wangpc on Jun 29 2023, 4:49 AM.
Details Summary A new Init type ArgumentInit is added to represent arguments, the The index of argument in error message is removed.
Diff Detail
Unit TestsFailed Event Timelinewangpc added a parent revision: D154065: [TableGen] Extract functions to resolve arguments [nfc].Jun 29 2023, 4:50 AM Comment Actions LGTM in general, with a nit.
This revision is now accepted and ready to land.Jun 29 2023, 12:45 PM Comment Actions Please collapse PositionalArgumentInit into ArgumentInit for this patch. As of this patch, there is no other kind, and the various dispatch logic is just confusing on it's own. The main value of this patch, and the reason I suggested it, was the type signature change. Having ArgumentInit in place allows you to split it's subclasses in the next change, and focus the review on that. Comment Actions
@reames I don't know if I understond your comment correctly, and I just put PositionalArgumentInit code in followed patch. This revision was landed with ongoing or failed builds.Jul 11 2023, 11:42 AM Closed by commit rG6251adc64d3d: [TableGen] Refactor the implementation of arguments to introduce ArgumentInit… (authored by wangpc, committed by reames). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions I went ahead and landed this change after applying a number of stylistic fixes. I also removed a bit of complexity which seems to exist only for the follow up patch, and is worthy of a bit of discussion there.
Revision Contents
Diff 538943 llvm/include/llvm/TableGen/Record.h
llvm/lib/TableGen/Record.cpp
llvm/lib/TableGen/TGParser.h
llvm/lib/TableGen/TGParser.cpp
|
changeValue() does not seem to match what the function actually does.
Change implies that we're modifying something already exsting. Based on the implementation, it appears that the function *creates* a new ArgumentInit with the current Index, and the new value. I think the name should reflect it. Perhaps cloneWithValue() would match the intent better?
Also, it appears to be implemented for the PositionalArgumentInit only. If it's needed for other types, then we may need a sensible default implementation. If it's not intended to be used in other circumstances, perhaps we can just construct the new PositionalArgumentInit explicitly in the CheckTemplateArgValues which is the only place where it's used, and drop the virtual function?