Remove libecl-style "type-safety" #4051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pinkwah
force-pushed
the
unsafe
branch
5 times, most recently
from
9707497
to
737b327
Compare
| @@ -1,8 +1,6 @@ | |||
| #ifndef ERT_SUMMARY_KEY_SET_H | |||
| #define ERT_SUMMARY_KEY_SET_H | |||
|
|
|||
| #include <ert/util/type_macros.h> | |||
|
|
|||
| #include <ert/enkf/enkf_types.hpp> | |||
There was a problem hiding this comment.
aparently stringlist_type was transiently imported from type_macros here. The fact that build/test succeeds means that ert/enkf/summary_key_set.hpp is never read.
eivindjahren
approved these changes
Oct 14, 2022
|
|
pinkwah
force-pushed
the
unsafe
branch
2 times, most recently
from
b2f64a3
to
9c8a87e
Compare
Old `clib` C classes provide `_safe_cast`, `_safe_cast_const` and `_is_instance`
functions that take a `void *` as input and cast it to the correct function, or
`util_abort` if the types are incorrect. This works because old-style structs
start with a `int __type_id` (`UTIL_TYPE_ID_DECLARATION` macro), and this number
is the magic `_TYPE_ID` for that type. Eg. `enkf_fs_type`'s TYPE_ID is `1089763`
-- a unique arbitrary number chosen when the type was first created. If the
`__type_id` matches the given TYPE_ID for a type, then the `_safe_cast`
functions pass.
There are multiple issues with this:
1. There is no guarantee that the `void *` object starts with `int __type_id`.
Running `enkf_fs_safe_cast("foo")` where the type is `const char *` is
undefined behaviour. So while the code may compile for a lot of pointers, it
might not be a safe operation (ie. "safe cast" isn't actually safe).
2. C++ is a strongly-typed language. Unlike C, there are no implicit pointer
casts (except _to_ `void *`. Notably this is why you need to cast `malloc()`
in C++ but not C). We should use the type system correctly and resolve any
type issues at compile-time rather than run-time. "Safe" casting consumes CPU
cycles when the correct solution would not.
3. libecl's types use type-erasure to make things behave in C. For example,
`vector_type` cannot know which type it contains, so it must rely on `void *`
to do the job. Therefore, a lot of interaction with libecl's API is using
`void *`. We are moving away from this approach, instead relying on C++
templated classes which do preserve the type.
This is a continuation of the previous commit. I remove the implementation rather than the uses in this.
Codecov Report
@@ Coverage Diff @@
## main #4051 +/- ##
==========================================
+ Coverage 57.70% 58.31% +0.60%
==========================================
Files 523 526 +3
Lines 39522 39161 -361
Branches 3592 3452 -140
==========================================
+ Hits 22808 22837 +29
+ Misses 15782 15445 -337
+ Partials 932 879 -53
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Old
clibC classes provide_safe_cast,_safe_cast_constand_is_instancefunctions that take avoid *as input and cast it to the correct function, orutil_abortif the types are incorrect. This works because old-style structs start with aint __type_id(UTIL_TYPE_ID_DECLARATIONmacro), and this number is the magic_TYPE_IDfor that type. Eg.enkf_fs_type's TYPE_ID is1089763-- a unique arbitrary number chosen when the type was first created. If the__type_idmatches the given TYPE_ID for a type, then the_safe_castfunctions pass.There are multiple issues with this:
void *object starts withint __type_id. Runningenkf_fs_safe_cast("foo")where the type isconst char *is undefined behaviour. So while the code may compile for a lot of pointers, it might not be a safe operation (ie. "safe cast" isn't actually safe).void *. Notably this is why you need to castmalloc()in C++ but not C). We should use the type system correctly and resolve any type issues at compile-time rather than run-time. "Safe" casting consumes CPU cycles when the correct solution would not.vector_typecannot know which type it contains, so it must rely onvoid *to do the job. Therefore, a lot of interaction with libecl's API is usingvoid *. We are moving away from this approach, instead relying on C++ templated classes which do preserve the type.