[PATCH] D106915: Add a DIExpression const-folder to prevent silly expressions

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

In D106915#2908862 <https://reviews.llvm.org/D106915#2908862>, @aprantl wrote:

> Thanks for looking into this! Is there any advantage to return a Constant+DIExpression, or could we just return a DIExpression(DW_OP_constu, ...)? Or can we not do that because even after folding the constant is > 64bits?

Looking at the rest of the infrastructure, I think the Constant is not optional, the DIExpression is the optional part.  Most of the time there isn't an expression at all, of course, just the initial operand (which might or might not be a Constant).  I suppose that could be redesigned but I think it goes beyond the scope of this patch.

> I think this new API would be a great candidate to create a unit test harness for (perhaps in MetadataTest.cpp).

You point out something I was thinking after I posted the patch, i.e. that the testing was just the sort I always grump about--it verifies the right thing happens in the case that prompts the patch, without looking at the broader cases.  Unit testing will be just the thing.


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

https://reviews.llvm.org/D106915