[PATCH] D115128: [TableGen][CodeEmitter] Introducing the VarLenCodeEmitterGen infrastructure

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:126
+      // Normalization: Hi bit should always be the second argument.
+      ArrayRef<Init *> NewArgs = {OperandName, LoBit, HiBit};
+      Segments.push_back(
----------------
myhsu wrote:
> craig.topper wrote:
> > This code is dangerous. It will create a dangling reference to an initializer_list that goes out of scope when the line ends. Use a plain C array here.
> I don't think it will create a dangling reference in this particular case because DagInit::get actually [[ https://github.com/llvm/llvm-project/blob/93fbaa46c82abd2d54bce9a7c3b39b01c30220d6/llvm/lib/TableGen/Record.cpp#L2120 | copies ]] NewArgs' elements. But I'm changing to plain C array anyway.
That's not Craig's point; `ArrayRef<Init *> NewArgs = {OperandName, LoBit, HiBit};` itself is broken, as NewArgs points to storage after that statement whose lifetime has ended (well, it's fine so long as you never use NewArgs, but then it's pointless), so by the time you've reached DagInit::get it's already too late, the storage is gone. Using an initialiser list for ArrayRef is solely for use within the same statement (i.e. things like `foo(ArrayRef({...}))`.


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

https://reviews.llvm.org/D115128