c-style casting to c++ #173
Conversation
smefpw
commented
Oct 19, 2019
smefpw
commented
> changes made to item_definitions > change some variables to const > [[nodiscard]] tag added to icon_override
| @@ -195,7 +195,7 @@ class config | |||
| return m_icon_overrides; | |||
| } | |||
|
|
|||
| auto get_icon_override(const std::string_view original) const -> const char* | |||
| [[nodiscard]] auto get_icon_override(const std::string_view original) const -> const char* | |||
There was a problem hiding this comment.
nodiscard is supposed to be used when discarding the result results in something bad, like memory leak
here it is just meaningless to do so, but not actively harmful
src/Utilities/vmt_smart_hook.hpp
Outdated
| @@ -90,7 +90,7 @@ class table_hook | |||
| auto hook_function(Fn hooked_fn, const std::size_t index) -> Fn | |||
| { | |||
| m_new_vmt[index] = (void*)(hooked_fn); | |||
| return (Fn)(m_old_vmt[index]); | |||
| return static_cast<Fn>(m_old_vmt[index]); | |||
There was a problem hiding this comment.
C casting is intentional in templates, since "Fn" could be a type that cannot be static casted to from void*, like a function type. Maybe MSVC allows this, but it shouldn't, by standard function and object pointers (like void*) are not castable to each other with static_cast
There was a problem hiding this comment.
Compiles and runs fine but from what you're saying reinterpret_cast may work? (Maybe dynamic_cast? But I'm not really sure what that one does)
There was a problem hiding this comment.
"compiles and runs on my machine" is not the same as standard compilant code.
according to standard conversion between pointer to object and pointer to function must be a reinterpret cast, and even then it is platform defined behavior. Clang also considers it an error:
https://godbolt.org/z/x6T_8M
Why would I (or anyone for that matter) write platform dependent and nonstandard code without any benefits?
Anyways, I've rewritten this in master to not use conversion across object ptr and function ptr, since it is platform defined behavior according to the standard
There was a problem hiding this comment.
Aight I'll keep this in mind
Thanks
