Segy field data cleanup - #636
Merged
Merged
Conversation
Specialized versions are not needed for the moment and we should be fine with more general functions. Partly reverses commits "Introduce segy_get_field_[type]" / "Introduce segy_set_field_[type]" to make tests to use just int. For current code it is safe because in current tests/internal functions all values are 4 bytes or less and there are no 4-bytes unsigned integers in current headers. General segy_get/set_field_int integer specialization is left as is for now as it is heavily used internally in segy.c code. We should aim to reconsider this before 2.0 release as otherwise it would become a permanent part of the C interface. General int specialization is mostly used in functions dealing with fields important for segy layout. These functions would require logic update as those fields have overriding "extended" field. For those general segy_get_field/segy_set_field would be used and it might present an opportunity to get rid of "int" specialization there completely. Why we might want to get rid of general segy_get/set_field_int integer specialization: - more general segy_get_field/segy_set_field function is available - having two ways in the interface to do the same thing is confusing - general "int" version might be dangerous in library source code as we now want to be very careful with types of fields we read. Now it exists purely for developer's convenience due to segyio 1.0 having only int fields. We should aim to use segy_get_field/segy_set_field where it ok to do so. Then we can decide whether it could be removed from the interface completely. Note that specialization would still be heavily used in tests/matlab/applications, but if we manage to get rid of it in the main code, it could become tests-only helper.
Function is exposed in the header only for "application", which are supposed to be test-only applications. We should avoid having test-only functions in the interface.
segy_get_field now requires segy_field_data passed as argument, instead of creating and returning it on its own. Given that library has been available for a long time and there exist C-library users only, consistency in the return functions is highly desirable. Only few functions that can't fail return value instead of error code. From the presence of .error member we know that segy_get_field can fail, so it makes sense for it to return the error code. That also allows us to have consistency between all get-set methods.
"Init" became impractical as the only thing that it inits now is the datatype. Solution could be simplified even more if we in future decide to make binary header-fields not 3200, but 0 based. Then "segy_field_datatype" might disappear altogether.
.datatype field is kind of pointless at the moment. - In C we can't easily force someone to set datatype the same moment when .value is set. So nothing prevents us from setting value to i16 and datatype to u32. - In theory we can simply pass segy_field_value as an argument instead of passing full segy_field_data. However it doesn't fit into the spirit of function, as without knowing .datatype we can't interpret value on its own and are forced to do it according to the field mapping table instead of the actual datatype. The best option seems to force .datatype being set by checking that it corresponds to the datatype retrieved from field mapping. Additionally fd doesn't have to be a pointer. We should treat that value as any other local primitive as it is not that big in size.
achaikou
force-pushed
the
segy_field_data_cleanup
branch
from
July 25, 2025 19:12
59aa6b6 to
b7837d5
Compare
yngve793
approved these changes
Jul 28, 2025
|
|
||
| static void printSegyTraceInfo( const char* buf ) { | ||
| int cdp, tsf, xl, il; | ||
| segy_get_field_i32( buf, SEGY_TR_ENSEMBLE, &cdp ); |
Contributor
There was a problem hiding this comment.
Type specific functions was added by POs request.
| switch ( fd->datatype ) { | ||
|
|
||
| case SEGY_SIGNED_INTEGER_8_BYTE: | ||
| memcpy( &(fd->value.i64), header + (fd->field_index -1), formatsize( fd->datatype ) ); |
Contributor
There was a problem hiding this comment.
-1 was used to reduce the number of required variables.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
C get/set field cleanup that came up before and during proof-of-concept implementation for tracemaps.
Seems like nothing of the removed code is needed as it still would be us who would parse and return all fields from all trace headers.