[PATCH] D28212: typeinfo: provide a partial implementation for Win32

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd commandeered this revision.
compnerd edited reviewers, added: smeenai; removed: compnerd.
compnerd added inline comments.


================
Comment at: include/typeinfo:100
+
+  static const char *__name(const struct type_info::__data *__data);
+  static size_t __hash(const struct type_info::__data *__data);
----------------
EricWF wrote:
> These should probably have `_LIBCPP_FUNC_VIS` visibility declarations attached to them.
Wont bother due to out-of-line definition


================
Comment at: include/typeinfo:101
+  static const char *__name(const struct type_info::__data *__data);
+  static size_t __hash(const struct type_info::__data *__data);
+  static int __compare(const struct type_info::__data* __lls,
----------------
EricWF wrote:
> Is there a reason why `__name` and `__hash` need wrappers? Can't we just provide out-of-line definitions for `name()` and `hash_code()` directly?
I don't remember why ... we can address that when we hit it.  Going to out-of-line them.


================
Comment at: include/typeinfo:200
 #endif
+#endif
 
----------------
EricWF wrote:
> Comment on `#endif` please.
Sure!


================
Comment at: include/typeinfo:109
 protected:
+#if defined(_LIBCPP_HAS_MS_ABI_TYPEINFO)
+#else
----------------
smeenai wrote:
> compnerd wrote:
> > EricWF wrote:
> > > Don't we need a constructor here?
> > No, we do not need a constructor since the emitted object is already a well formed object instance.
> I believe @EricWF was referring to the internal constructors that take a `const char *`. Do we need one of those for the Microsoft typeinfo?
No, no such constructor is needed with MS ABI AFAIK.  Construction of the type does not invoke a constructor and libc++ does not create instances of this.


================
Comment at: src/typeinfo.cpp:28-32
+  static constexpr const size_t fnv_offset_basis = 14695981039346656037;
+  static constexpr const size_t fnv_prime = 10995116282110;
+#else
+  static constexpr const size_t fnv_offset_basis = 2166136261;
+  static constexpr const size_t fnv_prime = 16777619;
----------------
EricWF wrote:
> compnerd wrote:
> > rnk wrote:
> > > compnerd wrote:
> > > > majnemer wrote:
> > > > > majnemer wrote:
> > > > > > Why make these static? Seems strange to use that storage duration.
> > > > > These literals are ill-formed, I think you need a ULL suffix here.
> > > > Oh, just to ensure that they are handled as literal constants.  I can drop the static if you like, since the compiler should do the right thing anyways.
> > > Isn't `constexpr const` redundant?
> > Yeah, intentional.  I should be using `_LIBCPP_CONSTEXPR` just incase the compiler doesn't support constexpr.
> All supported compiler provide `constexpr` when building the dylib, so you can assume we have it.
> 
> I have no strong objection to the redundancy, but I'm not opposed to removing either `const` or `constexpr`.
Removed `static` and `const`.


https://reviews.llvm.org/D28212