[libcxx-commits] [PATCH] D97802: [libc++] Fix incorrect typeinfo comparison on ARM64

[Shoaib Meenai via Phabricator via libcxx-commits]

smeenai added inline comments.


================
Comment at: libcxx/include/typeinfo:158-162
+// suffices to compare their addresses. Otherwise, if at least one of the
+// RTTIs can't be assumed to be unique, we must perform a deep string comparison
+// of the type names. Note that if one of the RTTIs is guaranteed unique and
+// the other one isn't, then both RTTIs are necessarily not to be considered
+// equal.
----------------
This reads a little confusingly, because the "Otherwise, if at least one of the RTTIs can't be assumed to be unique bit" makes you think that the case where on RTTI is unique and one isn't will do the deep string comparison, but then the next sentence changes the interpretation of that case. Maybe say "if both RTTIs can't be assumed to be unique", since the last sentence already handles the non-unique + unique case?


================
Comment at: libcxx/include/typeinfo:164
 //
-// Note that the compiler is the one setting (or unsetting) the high bit of
-// the pointer when it constructs the type_info, depending on whether it can
-// guarantee uniqueness for that specific type_info.
+// The intent of this design is to remove the need for weak symbols. Specifically,
+// if a type would normally have a default-visibility RTTI emitted as a weak
----------------
Thanks for explaining the motivation of the design and when it kicks in. I'd been casually curious about this before, and it's a neat design :)


================
Comment at: libcxx/include/typeinfo:259
         return true;
-      if (__is_type_name_unique(__lhs, __rhs))
+      else if (__is_type_name_unique(__lhs) != __is_type_name_unique(__rhs))
         return false;
----------------
I don't understand this. My reading of the two parameter `__is_type_name_unique` function was that it would return true if either typeinfo was unique, and that's also what you're doing in line 266 below. Over here, if both typeinfos are unique, this condition won't kick in, so wouldn't we incorrectly fall through to the strcmp below?

Also a nit: LLVM code style recommends not using an `else` after a return: https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return. I don't know if libc++ normally does this differently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97802