[demangler] Fix demangling of 'half'

[stuart]

Summary

Demangle 'Dh' as 'half' (as per GCC), and not 'decimal16' (which doesn't make sense, as there is no IEEE 754 decimal16 format).

The Itanium C++ ABI specification describes 'Dh' as "IEEE 754r half-precision floating point (16 bits)".

[stuart]

This comment was removed by stuart.

[stuart]

The pre-merge check failures relating to the failure to include "ItaniumDemangle.h" seem confusing to me. I cannot see how they would be caused by this change, itself: this looks on the surface to be a problem with the CI. Do I need to do anything about this?

[jyknight]

In D103833#2803811, @stuart wrote:

The pre-merge check failures relating to the failure to include "ItaniumDemangle.h" seem confusing to me. I cannot see how they would be caused by this change, itself: this looks on the surface to be a problem with the CI. Do I need to do anything about this?

I think this may be because you uploaded a diff without the full path or something...it somehow thinks that you renamed llvm/include/llvm/Demangle/llvm/ItaniumDemangle.h to include/llvm/Demangle/llvm/ItaniumDemangle.h. In "Download Raw Diff" it has, e.g.

--- a/llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ b/include/llvm/Demangle/ItaniumDemangle.h

If you fix the paths in the diff, I think that'll get rid of the spurious "reformat" comments on the unchanged lines, as well.

For the actual change: using the name half here sounds fine, though.

[stuart]

Reupload with no changes.

[stuart]

@jyknight, I have reuploaded the exact same diff with full context:

diff --git libcxxabi/src/demangle/ItaniumDemangle.h libcxxabi/src/demangle/ItaniumDemangle.h
--- libcxxabi/src/demangle/ItaniumDemangle.h
+++ libcxxabi/src/demangle/ItaniumDemangle.h
@@ -1,5713 +1,5713 @@
[...]
diff --git llvm/include/llvm/Demangle/ItaniumDemangle.h llvm/include/llvm/Demangle/ItaniumDemangle.h
index 4e3af0cb2649..78035c38ee50 100644
--- llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ llvm/include/llvm/Demangle/ItaniumDemangle.h
@@ -1,5713 +1,5713 @@
[...]

"Download raw diff" still downloads an adjusted version of the diff with the context removed, and with the paths mangled (ironically) by Differential.

The full context is visible in Differential's pretty-printing of the diff. I believe the context would not be visible had I not included it in the patch as submitted.

Perhaps the path of "llvm/include/llvm" is confusing it. This change does apply to both libcxxapi and llvm, but I would prefer not to split it up if there is no need.

@klimek, @chandlerc, would you have any advice? I've read the Code Reviews with Phabricator guide (and also the LLVM Developer Policy) and can't see anything there that would indicate that I'm doing anything wrong.

—

Update: I've tried uploading again with "llvm-project" added to both the "a" and "b" paths, with the result that "[ab]/llvm-project/llvm" and "[ab]/llvm-project/libcxxabi" are now getting stripped out instead.

Please help.

[stuart]

Reupload with no changes.

[stuart]

This comment was removed by stuart.

[jyknight]

A git diff generally has an extraneous path component [ab] first, e.g. your diff should look like this:

diff --git a/libcxxabi/src/demangle/ItaniumDemangle.h b/libcxxabi/src/demangle/ItaniumDemangle.h
index 4e3af0cb2649..78035c38ee50 100644
--- a/llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ b/llvm/include/llvm/Demangle/ItaniumDemangle.h

but your diff excerpt in the comment above is missing that extra directory prefix. I suspect you've got a local configuration file setting diff.noprefix = true -- and unfortunately, while omitting the a/b prefixes is nicer for humans, it confuses most git tooling, which expects it to be present -- including git itself, e.g. git apply.

[stuart]

Add a/ and b/ prefixes to diff.

[stuart]

In D103833#2810319, @jyknight wrote:

but your diff excerpt in the comment above is missing that extra directory prefix. I suspect you've got a local configuration file setting diff.noprefix = true

Good catch. I've fixed this.

I've long since forgotten why I ever added diff.noprefix = true to my ~/.gitconfig... I'm not sure it was humans, actually! If I ever need this again, I'll make sure I add it as a local setting for the repositories where I need it! :-)

Sorry, @klimek and @chandlerc, for the noise.

[jyknight]

(I note the new behavior also matches gcc)

[stuart]

In D103833#2810781, @jyknight wrote:

(I note the new behavior also matches gcc)

Good point. (Also, how I came to be aware of this, and what actually informed the choice of "half" rather than "binary16", for that matter.)

Would you agree that it makes sense to mention this in the commit message?

[stuart]

No chances since last time. Rebased to force a rebuild.

[stuart]

If I understand correctly, this needs review from the libc++abi group before I can push it. It's a fairly trivial change. Could someone review this please? (Is there anyone specifically I should add?)

libcxxabi/src/demangle/ItaniumDemangle.h

case 'e':
       return make<NameType>("decimal128");
     //                ::= Df   # IEEE 754r decimal floating point (32 bits)
     case 'f':
       First += 2;
       return make<NameType>("decimal32");
     //                ::= Dh   # IEEE 754r half-precision floating point (16 bits)
     case 'h':
       First += 2;
-      return make<NameType>("decimal16");
+      return make<NameType>("half");
     //                ::= Di   # char32_t
     case 'i':
       First += 2;
       return make<NameType>("char32_t");
     //                ::= Ds   # char16_t
     case 's':
       First += 2;
       return make<NameType>("char16_t");

llvm/include/llvm/Demangle/ItaniumDemangle.h

case 'e':
       return make<NameType>("decimal128");
     //                ::= Df   # IEEE 754r decimal floating point (32 bits)
     case 'f':
       First += 2;
       return make<NameType>("decimal32");
     //                ::= Dh   # IEEE 754r half-precision floating point (16 bits)
     case 'h':
       First += 2;
-      return make<NameType>("decimal16");
+      return make<NameType>("half");
     //                ::= Di   # char32_t
     case 'i':
       First += 2;
       return make<NameType>("char32_t");
     //                ::= Ds   # char16_t
     case 's':
       First += 2;
       return make<NameType>("char16_t");