Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add compile-time checks w.r.t. is_primary or existence of get_secondary_event() #3027

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add compile-time checks w.r.t. is_primary or existence of get_secondary_event() #3027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50c00b8
feat: more constexpr is good
muffgaga Dec 1, 2023
e217e3c
feat: more compile-time interface checking
muffgaga Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libnestutil/enum_bitfield.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ operator^=( Enum& lhs, Enum rhs )
}

template < typename Enum >
bool
bool constexpr
flag_is_set( const Enum en, const Enum flag )
{
using underlying = typename std::underlying_type< Enum >::type;
Expand Down
15 changes: 0 additions & 15 deletions nestkernel/connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,14 +128,6 @@ class Connection
Connection( const Connection< targetidentifierT >& rhs ) = default;
Connection& operator=( const Connection< targetidentifierT >& rhs ) = default;

/**
* Get a pointer to an instance of a SecondaryEvent if this connection supports secondary events.
*
* To prevent erronous calls of this function on primary connections, the base class implementation
* below just contains `assert(false)`.
*/
SecondaryEvent* get_secondary_event();

/**
* Get all properties of this connection and put them into a dictionary.
*/
Expand Down Expand Up @@ -398,13 +390,6 @@ Connection< targetidentifierT >::trigger_update_weight( const size_t,
throw IllegalConnection( "Connection does not support updates that are triggered by a volume transmitter." );
}

template < typename targetidentifierT >
SecondaryEvent*
Connection< targetidentifierT >::get_secondary_event()
{
assert( false );
}

} // namespace nest

#endif /* CONNECTION_H */
36 changes: 34 additions & 2 deletions nestkernel/connector_model.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,26 @@ enum class ConnectionModelProperties : unsigned
REQUIRES_CLOPATH_ARCHIVING = 1 << 6,
REQUIRES_URBANCZIK_ARCHIVING = 1 << 7
};
}

namespace
{ // FIXME: utils namespace?

template < typename, typename = void >
struct has_get_secondary_event_t : std::false_type
{
};

template < typename ConnectionT >
struct has_get_secondary_event_t< ConnectionT,
std::void_t< decltype( std::declval< ConnectionT >().get_secondary_event() ) > > : public std::true_type
{
};

} // end of FIXME

namespace nest
{

template <>
struct EnableBitMaskOperators< ConnectionModelProperties >
Expand Down Expand Up @@ -128,7 +148,7 @@ class ConnectorModel
return name_;
}

bool
bool constexpr
has_property( const ConnectionModelProperties& property ) const
{
return flag_is_set( properties_, property );
Expand Down Expand Up @@ -199,7 +219,19 @@ class GenericConnectorModel : public ConnectorModel
SecondaryEvent*
get_secondary_event() override
{
return default_connection_.get_secondary_event();
constexpr bool is_primary = flag_is_set( ConnectionT::properties, nest::ConnectionModelProperties::IS_PRIMARY );
constexpr bool has_get_secondary_event = has_get_secondary_event_t< ConnectionT >::value;
static_assert(
is_primary xor has_get_secondary_event, "Non-primary connections have to provide get_secondary_event()" );
if constexpr ( ( not is_primary ) and has_get_secondary_event )
Copy link
Contributor

@otcathatsya otcathatsya Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be sufficient to only check one of the conditions after the assert

Edit: if this was accidentally called on a primary event synapse, would it be possible to reach the nullptr? Would it be safer to provide an error message there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be sufficient, but also doesn't hurt to check for both at compile time, right?

{
return default_connection_.get_secondary_event();
}
else
{
// unreachable code
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nullptr;
UnsupportedEvent( "Using secondary events on primary connections is not possible. Secondary connections must implement get_secondary_event!" );

}
}

ConnectionT const&
Expand Down
5 changes: 3 additions & 2 deletions nestkernel/connector_model_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,9 @@ GenericConnectorModel< ConnectionT >::clone( std::string name, synindex syn_id )
ConnectorModel* new_cm = new GenericConnectorModel( *this, name ); // calls copy construtor
new_cm->set_syn_id( syn_id );

const bool is_primary = new_cm->has_property( ConnectionModelProperties::IS_PRIMARY );
if ( not is_primary )
constexpr bool is_primary = flag_is_set( ConnectionT::properties, nest::ConnectionModelProperties::IS_PRIMARY );
constexpr bool has_get_secondary_event = has_get_secondary_event_t< ConnectionT >::value;
if constexpr ( ( not is_primary ) and has_get_secondary_event )
{
new_cm->get_secondary_event()->add_syn_id( syn_id );
}
Expand Down
Loading