[PATCH] D33892: Align definition of DW_OP_plus with DWARF spec [1/3]

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:553
+
+    if (FromVersion <= 2) {
+      // Change DW_OP_plus to DW_OP_plus_uconst.
----------------
aprantl wrote:
> sdesmalen wrote:
> > I'm starting with upgrading from version 2 to 0 (rather than in reverse order), so that we only need to do the 'copy' to NewExpr once.
> I'm not sure I'm comfortable with reversing the order. In the original implementation we incrementally upgrade the expression from version N-1 to version N. Some of the upgrades (including the one that is added in this patch!) are changing how to iterate over the elements, so it is not generally safe to apply the upgrades in reverse order. If we are upgrading in chronological order then a new upgrade doesn't have to care about the old upgrades, but in the reverse order they have to inspect all other upgrades if they are still correct.
> 
> I recommend to leave the order as in the original implementation. I wouldn't worry too much about performance — the upgrade path is not the normal use-case.
Valid point about changing the order of applying updates!
I've changed it back to the original order.


https://reviews.llvm.org/D33892