[PATCH] D106585: Fix clang debug info irgen of i128 enums

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3117
+    llvm::APSInt Value = Enum->getInitVal();
+    Value.setIsSigned(IsSigned);
+    Enumerators.push_back(DBuilder.createEnumerator(Enum->getName(), Value));
----------------
dblaikie wrote:
> Is the value already signed appropriately?
> 
> Removing this line of code (and the `bool IsSigned` variable, so it doesn't break `-Werror=unused-variable`) doesn't cause any tests to fail, that I can see.
I'm afraid it might not be NFC. I took the cautious approach of trying to leave things exactly as they were. Enums in C can have surprisingly different behavior, IIRC.


================
Comment at: llvm/lib/IR/DIBuilder.cpp:255
+  assert(!Name.empty() && "Unable to create enumerator without name");
+  return DIEnumerator::get(VMContext, APInt(Value), Value.isUnsigned(), Name);
+}
----------------
mizvekov wrote:
> Do I get it right, and this is not the first place that I noticed this, but the terminology here is a bit unconventional with regards to "Signed" vs "Negative"?
> 
> It looks like around the debug info code, an APSInt will be a signed positive value for representing a negative number, and an unsigned one to represent a positive value.
To my knowledge, we try not to store data in a sign-and-magnitude representation, it's always twos-complement. Meaning, APInt always stores a sequence of bits, typically interpreted as an unsigned positive integer. With outside information about the signedness of that data, you can answer the question of whether it is negative or not. APSInt adds the extra "signed" field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106585