[PATCH] D78672: [Debuginfo] Remove redundand variable from getAttributeValue()

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

I'd be inclined to rephrase this a bit & actually use MatchAttrIndex more rather than less - to make the fast path clearer/more integrated, rather than less (given @clayborg's concerns).

For instance, that "return None" at the end of the function should be unreachable, right? We should never make it through the for loop without at some point satisfying the condition... oh, but we would if we failed to extract the value - but if we did, should we keep going looking for other instances of the same attribute? (the old code kept looping, but would never find another, the new code keeps looping and might find another instance of the attribute - the DWARF spec says that's invalid, but I'm not sure any of our parsers enforce that? Maybe LLDB's does?)

I'm imagining something more like:

  Optional<uint32_t> MatchAttrIndex = findAttributeIndex(Attr);
  if (!MatchAttrIndex)
    return None;
  
  for (int i = 0; i != *MatchAttrIndex; ++i) {
    if (auto FixedSize = Spec.getByteSize(U))
      Offset += *FixedSize;
    else
      DWARFFormValue::skipValue(...);
  }
  
  // We have arrived at the attribute to extract, extract if from Offset.
  if (Spec.isImplicitConst())
    return DWARFFormValue::createFromSValue(...);
  
  DWARFFormValue FormValue(Spec.Form);
  if (FormValue.extractValue(DebugInfoData, &Offset, U.getFormParams(), &U))
    return FormValue;
  
  return None;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78672