Skip to content

Commit ba23862

Browse files
AdamGlusteinSimon Beyzerov
andauthored
Add unset field semantics for structs ("strict structs") (#607)
* Add unset field semantics (disc #536) Signed-off-by: Simon Beyzerov <[email protected]> * Fix up PR Signed-off-by: Adam Glustein <[email protected]> * Further clean up of PR; add docs, improve error messages, fix inheritance bug Signed-off-by: Adam Glustein <[email protected]> * Change lint to use 3.11 Signed-off-by: Adam Glustein <[email protected]> * Fix 3.10+ Union annotations Signed-off-by: Adam Glustein <[email protected]> * Fix typo in docs Signed-off-by: Adam Glustein <[email protected]> * Store optionality on the field object, and embed whether or not the field is set to None in the struct's bitmask Signed-off-by: Adam Glustein <[email protected]> * More PR comments Signed-off-by: Adam Glustein <[email protected]> * Fix str/repr for strict structs Signed-off-by: Adam Glustein <[email protected]> * WIP PR comments Signed-off-by: Adam Glustein <[email protected]> * PR comments Signed-off-by: Adam Glustein <[email protected]> * Store set and none masks in the same vector, provide accessors Signed-off-by: Adam Glustein <[email protected]> * Tests: add collectts test case, merge wiring_access tests, add fromts test Signed-off-by: Adam Glustein <[email protected]> * Update pydantic schema Signed-off-by: Adam Glustein <[email protected]> * Add tests for pydantic validation Signed-off-by: Adam Glustein <[email protected]> --------- Signed-off-by: Simon Beyzerov <[email protected]> Signed-off-by: Adam Glustein <[email protected]> Co-authored-by: Simon Beyzerov <[email protected]>
1 parent 9397ea1 commit ba23862

File tree

14 files changed

+856
-93
lines changed

14 files changed

+856
-93
lines changed

cpp/csp/cppnodes/baselibimpl.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -705,6 +705,9 @@ DECLARE_CPPNODE( struct_fromts )
705705
);
706706
}
707707

708+
if( unlikely( !out.get() -> validate() ) )
709+
CSP_THROW( ValueError, "Struct " << cls.value() -> name() << " is not valid; required fields " << out -> formatAllUnsetStrictFields() << " did not tick" );
710+
708711
CSP_OUTPUT( std::move( out ) );
709712
}
710713

@@ -758,6 +761,9 @@ DECLARE_CPPNODE( struct_collectts )
758761
}
759762
);
760763
}
764+
765+
if( unlikely( !out.get() -> validate() ) )
766+
CSP_THROW( ValueError, "Struct " << cls.value() -> name() << " is not valid; required fields " << out -> formatAllUnsetStrictFields() << " did not tick" );
761767

762768
CSP_OUTPUT( std::move( out ) );
763769
}

cpp/csp/engine/InputAdapter.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
#include <csp/core/Time.h>
55
#include <csp/engine/Enums.h>
66
#include <csp/engine/RootEngine.h>
7+
#include <csp/engine/Struct.h>
78
#include <csp/engine/TimeSeriesProvider.h>
89

910
namespace csp
@@ -22,6 +23,8 @@ class InputAdapter : public TimeSeriesProvider, public EngineOwned
2223
virtual void start( DateTime start, DateTime end ) {}
2324
virtual void stop() {}
2425

26+
virtual const char * name() const { return "InputAdapter"; }
27+
2528
template< typename T > void outputTickTyped( DateTime timestamp, const T & value )
2629
{
2730
outputTickTyped( rootEngine() -> cycleCount(), timestamp, value );
@@ -55,6 +58,13 @@ class InputAdapter : public TimeSeriesProvider, public EngineOwned
5558
template<typename T>
5659
bool InputAdapter::consumeTick( const T & value )
5760
{
61+
if constexpr( CspType::Type::fromCType<T>::type == CspType::TypeTraits::STRUCT )
62+
{
63+
if( unlikely( !( value -> validate() ) ) )
64+
CSP_THROW( ValueError, "Struct " << value -> meta() -> name() << " from adapter type " << name()
65+
<< " is not valid; required fields " << value -> formatAllUnsetStrictFields() << " were not set on init" );
66+
}
67+
5868
switch( pushMode() )
5969
{
6070
case PushMode::LAST_VALUE:

cpp/csp/engine/Struct.cpp

Lines changed: 107 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,26 @@
11
#include <csp/core/System.h>
22
#include <csp/engine/Struct.h>
33
#include <algorithm>
4+
#include <ranges>
5+
#include <string>
6+
#include <sstream>
47

58
namespace csp
69
{
710

811
StructField::StructField( CspTypePtr type, const std::string & fieldname,
9-
size_t size, size_t alignment ) :
12+
size_t size, size_t alignment, bool isOptional ) :
1013
m_fieldname( fieldname ),
1114
m_offset( 0 ),
1215
m_size( size ),
1316
m_alignment( alignment ),
1417
m_maskOffset( 0 ),
1518
m_maskBit( 0 ),
19+
m_noneMaskBit( 0 ),
1620
m_maskBitMask( 0 ),
17-
m_type( type )
21+
m_noneMaskBitMask( 0 ),
22+
m_type( type ),
23+
m_isOptional( isOptional )
1824
{
1925
}
2026

@@ -33,13 +39,13 @@ and adjustments required for the hidden fields
3339
3440
*/
3541

36-
StructMeta::StructMeta( const std::string & name, const Fields & fields,
37-
std::shared_ptr<StructMeta> base ) : m_name( name ), m_base( base ), m_fields( fields ),
42+
StructMeta::StructMeta( const std::string & name, const Fields & fields, bool isStrict,
43+
std::shared_ptr<StructMeta> base ) : m_name( name ), m_base( base ), m_fields( fields ), m_optionalFieldsBitMasks(),
3844
m_size( 0 ), m_partialSize( 0 ), m_partialStart( 0 ), m_nativeStart( 0 ), m_basePadding( 0 ),
3945
m_maskLoc( 0 ), m_maskSize( 0 ), m_firstPartialField( 0 ), m_firstNativePartialField( 0 ),
40-
m_isPartialNative( true ), m_isFullyNative( true )
46+
m_isPartialNative( true ), m_isFullyNative( true ), m_isStrict( isStrict )
4147
{
42-
if( m_fields.empty() && !m_base)
48+
if( m_fields.empty() && !m_base )
4349
CSP_THROW( TypeError, "Struct types must define at least 1 field" );
4450

4551
//sort by sizes, biggest first, to get proper alignment
@@ -95,25 +101,64 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields,
95101
//Setup masking bits for our fields
96102
//NOTE we can be more efficient by sticking masks into any potential alignment gaps, dont want to spend time on it
97103
//at this point
98-
m_maskSize = !m_fields.empty() ? 1 + ( ( m_fields.size() - 1 ) / 8 ) : 0;
104+
105+
size_t optionalFieldCount = std::count_if( m_fields.begin(), m_fields.end(), []( const auto & f ) { return f -> isOptional(); } );
106+
107+
m_maskSize = !m_fields.empty() ? 1 + ( ( m_fields.size() + optionalFieldCount - 1 ) / 8 ) : 0;
99108
m_size = offset + m_maskSize;
100109
m_partialSize = m_size - baseSize;
101110
m_maskLoc = m_size - m_maskSize;
111+
112+
uint8_t numRemainingBits = ( m_fields.size() + optionalFieldCount ) % 8;
113+
m_lastByteMask = ( 1u << numRemainingBits ) - 1;
102114

103115
size_t maskLoc = m_maskLoc;
104116
uint8_t maskBit = 0;
105-
for( auto & f : m_fields )
117+
118+
// Set optional fields first so that their 2-bits never cross a byte boundary
119+
// Put both the set bits and none bits in the same vector to avoid fragmentation
120+
m_optionalFieldsBitMasks.resize( 2 * m_maskSize );
121+
for( size_t i = 0; i < m_fields.size(); ++i )
122+
{
123+
auto & f = m_fields[ i ];
124+
if( f -> isOptional() )
125+
{
126+
f -> setMaskOffset( maskLoc, maskBit );
127+
optionalSetBitMask( maskLoc - m_maskLoc ) |= ( 1 << maskBit ); // set bits for this byte
128+
optionalNoneBitMask( maskLoc - m_maskLoc ) |= ( 1 << ++maskBit ); // none bits for this byte
129+
if( ++maskBit == 8 )
130+
{
131+
maskBit = 0;
132+
++maskLoc;
133+
}
134+
}
135+
}
136+
137+
for( size_t i = 0; i < m_fields.size(); ++i )
106138
{
107-
f -> setMaskOffset( maskLoc, maskBit );
108-
if( ++maskBit == 8 )
139+
auto & f = m_fields[ i ];
140+
if( !f -> isOptional() )
109141
{
110-
maskBit = 0;
111-
++maskLoc;
142+
f -> setMaskOffset( maskLoc, maskBit );
143+
if( ++maskBit == 8 )
144+
{
145+
maskBit = 0;
146+
++maskLoc;
147+
}
112148
}
113149
}
114150

115151
if( m_base )
116152
{
153+
// The complete inheritance hierarchy must agree on strict/non-strict
154+
if( m_isStrict != m_base -> isStrict() )
155+
{
156+
CSP_THROW( ValueError,
157+
"Struct " << m_name << " was declared " << ( m_isStrict ? "strict" : "non-strict" ) << " but derives from "
158+
<< m_base -> name() << " which is " << ( m_base -> isStrict() ? "strict" : "non-strict" )
159+
);
160+
}
161+
117162
m_fields.insert( m_fields.begin(), m_base -> m_fields.begin(), m_base -> m_fields.end() );
118163
m_fieldnames.insert( m_fieldnames.begin(), m_base -> m_fieldnames.begin(), m_base -> m_fieldnames.end() );
119164

@@ -453,23 +498,60 @@ void StructMeta::clear( Struct * s ) const
453498
m_base -> clear( s );
454499
}
455500

501+
std::string StructMeta::formatAllUnsetStrictFields( const Struct * s ) const
502+
{
503+
bool first = true;
504+
std::stringstream ss;
505+
ss << "[";
506+
507+
for( size_t i = 0; i < m_fields.size(); ++i )
508+
{
509+
const auto & f = m_fields[ i ];
510+
if( !f -> isSet( s ) && !f -> isNone( s ) )
511+
{
512+
if( !first )
513+
ss << ", ";
514+
else
515+
first = false;
516+
ss << f -> fieldname();
517+
}
518+
}
519+
ss << "]";
520+
return ss.str();
521+
}
522+
456523
bool StructMeta::allFieldsSet( const Struct * s ) const
457524
{
458-
size_t numLocalFields = m_fields.size() - m_firstPartialField;
459-
uint8_t numRemainingBits = numLocalFields % 8;
525+
/*
526+
We can use bit operations to validate the struct.
527+
1. Let M1 be the bitmask with 1s that align with the set bit of optional fields and
528+
2. Let M2 be the bitmask with 1s that align with the none bit of optional fields.
529+
-- Both M1 and M2 are computed trivially when we create the meta.
530+
3. Let M1* = M1 & mask. M1* now is the bitmask of all set optional fields.
531+
4. Similarly, let M2* = M2 & mask, such that M2* is the bitmask of all none optional fields.
532+
5. Let M3 = mask | (M1* << 1) | (M2* >> 1). Since the shifted set/none bitsets will fill in that optional fields other bit,
533+
the struct can validate iff M3 is all 1s.
534+
535+
Notes:
536+
1) We do this on a byte by byte basis currently. If we add 32/64 bit padding to the struct mask, we could do this as one single step for most structs.
537+
2) There is an edge condition if a) the set bit of an optional field is the last bit of a byte or b) the none bit of an optional field is the first bit of a byte.
538+
So, when we create the meta, we ensure this never happens by being smart about the ordering of fields in the mask.
539+
*/
460540

461541
const uint8_t * m = reinterpret_cast<const uint8_t *>( s ) + m_maskLoc;
462-
const uint8_t * e = m + m_maskSize - bool( numRemainingBits );
463-
for( ; m < e; ++m )
542+
const uint8_t * e = m + m_maskSize - bool( m_lastByteMask );
543+
544+
size_t i = 0;
545+
for( ; m < e; ++m, ++i )
464546
{
465-
if( *m != 0xFF )
547+
if( ( *m | ( ( *m & optionalSetBitMask( i ) ) << 1 ) | ( ( *m & optionalNoneBitMask( i ) ) >> 1 ) ) != 0xFF )
466548
return false;
467549
}
468550

469-
if( numRemainingBits > 0 )
551+
if( m_lastByteMask )
470552
{
471-
uint8_t bitmask = ( 1u << numRemainingBits ) - 1;
472-
if( ( *m & bitmask ) != bitmask )
553+
uint8_t masked = *m & m_lastByteMask;
554+
if( ( masked | ( ( ( masked & optionalSetBitMask( i ) ) << 1 ) & m_lastByteMask ) | ( ( masked & optionalNoneBitMask( i ) ) >> 1 ) ) != m_lastByteMask )
473555
return false;
474556
}
475557

@@ -494,6 +576,11 @@ void StructMeta::destroy( Struct * s ) const
494576
m_base -> destroy( s );
495577
}
496578

579+
[[nodiscard]] bool StructMeta::validate( const Struct * s ) const
580+
{
581+
return !isStrict() || allFieldsSet( s );
582+
}
583+
497584
Struct::Struct( const std::shared_ptr<const StructMeta> & meta )
498585
{
499586
//Initialize meta shared_ptr

0 commit comments

Comments
 (0)