[PATCH] D43563: TableGen: Get rid of Init::getFieldInit

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/TableGen/Record.cpp:1377
 Init *FieldInit::resolveReferences(Record &R, const RecordVal *RV) const {
-  Init *NewRec = RV ? Rec->resolveReferences(R, RV) : Rec;
+  Init *NewRec = Rec->resolveReferences(R, RV);
 
----------------
nhaehnle wrote:
> tra wrote:
> > assert(RV) as we no longer allow it to be nullptr.
> Actually no, this is on purpose. resolveReferences works fine with RV == nullptr, it just has subtly different semantics (resolving via R.getValue() instead of resolving only RV). All the other recursive resolving calls look like this as well, e.g. when resolving the list in a VarListElementInit or operands of OpInit subclasses. The whole point of this change is to streamline the resolving process in FieldInit as well.
> 
> I haven't looked into the full history of this, but I suspect somebody mistakenly decided to skip the recursive resolveReferences call in the !RV call, and then somebody else added the special code in VarInit::getFieldInit to fix one particular case that ended up being broken by the lack of resolve calls.
> 
> Whatever the history, the Right Thing to do is for all composite Inits to recursively resolve their components in a standardized way. I have an upcoming change that does a similar thing with BitsInit.
That should have read: ... mistakenly decided to skip the recursive resolveReferences call in the !RV **case**.

Point is, VarInit::getFieldInit papers over this mistake in some specific cases but not all.


Repository:
  rL LLVM

https://reviews.llvm.org/D43563