[PATCH] D99273: [DebugInfo] Support for signed constants inside DIExpression

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 2 inline comments as done.
SouraVX added inline comments.


================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1472
+       getElement(2) != dwarf::DW_OP_stack_value))
     return false;
   return true;
----------------
aprantl wrote:
> aprantl wrote:
> > SouraVX wrote:
> > > aprantl wrote:
> > > > dstenb wrote:
> > > > > Why do we not allow fragmented expressions here but we do for the unsigned variant?
> > > > It's costly to maintain two function that share 95% of their implementation. What would you think about having a single function
> > > > 
> > > > `llvm::Optional<enum {unsigned, signed}> DIExpression::isConstant() const;`
> > > > 
> > > > instead? Then it can serve as a drop-in replacements for call sites that don't care about the signedness and ones that do still get that information.
> > > > 
> > > Thanks for the suggestion! I went a little ahead and did it in a more functional way, preserving the semantics in individual lambdas.
> > > Please let me know, WDYT ?
> > You implemented my suggestion very literally. Ideally we would share the *implementation* of the two lambdas as well, for example, by inverting the condition and use early exists:
> > 
> > ```
> > if ((getNumElements() == 2 && (getElement(0) != dwarf::DW_OP_consts))
> >   return true;
> > if  (getNumElements() != 3_
> >   return false;
> > if ((getElement(0) != dwarf::DW_OP_consts || (getElement(0) != dwarf::DW_OP_constu))
> >   return false;
> > return getElement(2) != dwarf::DW_OP_stack_value);
> > ```
> > ...
> > 
> > 
> should have been 
> `if ((getElement(0) != dwarf::DW_OP_consts && (getElement(0) != dwarf::DW_OP_constu))`
I've tried to follow what you meant :)
Changes:
- De-duplicated/factored out common code.
- Arranged expensive checks first, this should help in short-circuiting in case of malformed expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99273