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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
