Skip to content

fix(AttributeManager): remove import method taking span #1322

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

Open
BenPinet wants to merge 3 commits into
base: v18
Choose a base branch
from
Open

fix(AttributeManager): remove import method taking span #1322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dcf157e
fix(AttributeManager): remove import method taking span
BenPinet Aug 14, 2026
dc44b49
Merge branch 'v18' into fix/remove_import_method_taking_span
BenPinet Aug 14, 2026
6cb3943
Merge branch 'v18' into fix/remove_import_method_taking_span
BenPinet Aug 14, 2026
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
4 changes: 0 additions & 4 deletions include/geode/basic/attribute.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
/*!
* Base class defining the virtual API used by the AttributeManager.
*/
class AttributeBase : public Identifier

Check warning on line 52 in include/geode/basic/attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/attribute.hpp:52:11 [cppcoreguidelines-special-member-functions] 

class 'AttributeBase' defines a default destructor, a copy constructor and a copy assignment operator but does not define a move constructor or a move assignment operator
{
OPENGEODE_DISABLE_COPY( AttributeBase );
friend class bitsery::Access;
Expand Down Expand Up @@ -79,7 +79,7 @@

void set_properties( AttributeProperties new_properties )
{
properties_ = std::move( new_properties );

Check warning on line 82 in include/geode/basic/attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/attribute.hpp:82:27 [hicpp-move-const-arg] 

std::move of the variable 'new_properties' of the trivially-copyable type 'AttributeProperties' has no effect; remove std::move()
}

public:
Expand All @@ -100,10 +100,6 @@
index_t nb_elements,
AttributeKey /*key*/ ) const = 0;

virtual void import( absl::Span< const index_t > old2new,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) = 0;

virtual void import( const GenericMapping< index_t >& old2new_mapping,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) = 0;
Expand Down Expand Up @@ -153,7 +149,7 @@

protected:
AttributeBase( std::string_view name, AttributeProperties properties )
: properties_( std::move( properties ) )

Check warning on line 152 in include/geode/basic/attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/attribute.hpp:152:28 [hicpp-move-const-arg] 

std::move of the variable 'properties' of the trivially-copyable type 'AttributeProperties' has no effect; remove std::move()
{
set_name( name );
}
Expand Down
7 changes: 0 additions & 7 deletions include/geode/basic/attribute_manager.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,13 +252,6 @@ namespace geode

void copy( const AttributeManager& attribute_manager );

void import( const AttributeManager& attribute_manager,
absl::Span< const index_t > old2new );

void import( const AttributeManager& attribute_manager,
absl::Span< const index_t > old2new,
const uuid& attribute_id );

void import( const AttributeManager& attribute_manager,
const GenericMapping< index_t >& old2new_mapping );

Expand Down
7 changes: 0 additions & 7 deletions include/geode/basic/constant_attribute.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
set_value( std::move( value ) );
}

ConstantAttribute( std::string_view name )

Check warning on line 121 in include/geode/basic/constant_attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/constant_attribute.hpp:121:9 [google-explicit-constructor] 

single-argument constructors must be marked explicit to avoid unintentional implicit conversions
: ReadOnlyAttribute< T >( name, AttributeProperties{} ) {};

ConstantAttribute()
Expand Down Expand Up @@ -203,13 +203,6 @@
return attribute;
}

void import( absl::Span< const index_t > /* unused */,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) override
{
import( dynamic_cast< const ReadOnlyAttribute< T >& >( *from ) );
}

void import( const GenericMapping< index_t >& /* unused */,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) override
Expand Down
22 changes: 0 additions & 22 deletions include/geode/basic/sparse_attribute.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
{
if( value( element ) == default_values_.no_value )
{
return false;

Check warning on line 89 in include/geode/basic/sparse_attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/sparse_attribute.hpp:89:24 [readability-simplify-boolean-expr] 

redundant boolean literal in conditional return statement
}
return true;
}
Expand Down Expand Up @@ -131,10 +131,10 @@
: ReadOnlyAttribute< T >( name, std::move( properties ) ),
default_values_( std::move( default_values ) )
{
values_.reserve( 10 );

Check warning on line 134 in include/geode/basic/sparse_attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/sparse_attribute.hpp:134:30 [cppcoreguidelines-avoid-magic-numbers] 

10 is a magic number; consider replacing it with a named constant
}

SparseAttribute( std::string_view name )

Check warning on line 137 in include/geode/basic/sparse_attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/sparse_attribute.hpp:137:9 [google-explicit-constructor] 

single-argument constructors must be marked explicit to avoid unintentional implicit conversions
: ReadOnlyAttribute< T >( name, AttributeProperties{} )
{
}
Expand All @@ -161,7 +161,7 @@
archive.ext( attribute.values_,
bitsery::ext::StdMap{
attribute.values_.max_size() },
[]( Archive& archive2, index_t& i, T& item ) {

Check warning on line 164 in include/geode/basic/sparse_attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/sparse_attribute.hpp:164:62 [readability-identifier-length] 

parameter name 'i' is too short, expected at least 3 characters
archive2.value4b( i );
archive2( item );
} );
Expand All @@ -175,12 +175,12 @@
archive.ext( attribute.values_,
bitsery::ext::StdMap{
attribute.values_.max_size() },
[]( Archive& archive2, index_t& i, T& item ) {

Check warning on line 178 in include/geode/basic/sparse_attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/sparse_attribute.hpp:178:65 [readability-identifier-length] 

parameter name 'i' is too short, expected at least 3 characters
archive2.value4b( i );
archive2( item );
} );
} } } );
values_.reserve( 10 );

Check warning on line 183 in include/geode/basic/sparse_attribute.hpp

View workflow job for this annotation

GitHub Actions / test / tidy

include/geode/basic/sparse_attribute.hpp:183:30 [cppcoreguidelines-avoid-magic-numbers] 

10 is a magic number; consider replacing it with a named constant
}

void resize(
Expand Down Expand Up @@ -311,14 +311,6 @@
return attribute;
}

void import( absl::Span< const index_t > old2new,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) override
{
import( old2new,
dynamic_cast< const ReadOnlyAttribute< T >& >( *from ) );
}

void import( const GenericMapping< index_t >& old2new_mapping,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) override
Expand All @@ -327,20 +319,6 @@
dynamic_cast< const ReadOnlyAttribute< T >& >( *from ) );
}

void import( absl::Span< const index_t > old2new,
const ReadOnlyAttribute< T >& from )
{
for( const auto i : Indices{ old2new } )
{
const auto new_index = old2new[i];
if( from.value( i ) != default_values_.default_value
&& new_index != NO_ID )
{
this->set_value( new_index, from.value( i ) );
}
}
}

void import( const GenericMapping< index_t >& old2new_mapping,
const ReadOnlyAttribute< T >& from )
{
Expand Down
42 changes: 0 additions & 42 deletions include/geode/basic/variable_attribute.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,14 +287,6 @@ namespace geode
return attribute;
}

void import( absl::Span< const index_t > old2new,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) override
{
import( old2new,
dynamic_cast< const ReadOnlyAttribute< T >& >( *from ) );
}

void import( const GenericMapping< index_t >& old2new_mapping,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) override
Expand All @@ -303,19 +295,6 @@ namespace geode
dynamic_cast< const ReadOnlyAttribute< T >& >( *from ) );
}

void import( absl::Span< const index_t > old2new,
const ReadOnlyAttribute< T >& from )
{
for( const auto i : Indices{ old2new } )
{
const auto new_index = old2new[i];
if( new_index != NO_ID )
{
this->set_value( new_index, from.value( i ) );
}
}
}

void import( const GenericMapping< index_t >& old2new_mapping,
const ReadOnlyAttribute< T >& from )
{
Expand Down Expand Up @@ -569,14 +548,6 @@ namespace geode
return attribute;
}

void import( absl::Span< const index_t > old2new,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) override
{
import( old2new,
dynamic_cast< const ReadOnlyAttribute< bool >& >( *from ) );
}

void import( const GenericMapping< index_t >& old2new_mapping,
const std::shared_ptr< AttributeBase >& from,
AttributeBase::AttributeKey /*key*/ ) override
Expand All @@ -585,19 +556,6 @@ namespace geode
dynamic_cast< const ReadOnlyAttribute< bool >& >( *from ) );
}

void import( absl::Span< const index_t > old2new,
const ReadOnlyAttribute< bool >& from )
{
for( const auto i : Indices{ old2new } )
{
const auto new_index = old2new[i];
if( new_index != NO_ID )
{
this->set_value( new_index, from.value( i ) );
}
}
}

void import( const GenericMapping< index_t >& old2new_mapping,
const ReadOnlyAttribute< bool >& from )
{
Expand Down
15 changes: 0 additions & 15 deletions src/geode/basic/attribute_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,21 +570,6 @@ namespace geode
return impl_->attribute_ids_matching_name( name );
}

void AttributeManager::import( const AttributeManager &attribute_manager,
absl::Span< const index_t > old2new )
{
impl_->import(
*attribute_manager.impl_, old2new, AttributeBase::AttributeKey{} );
}

void AttributeManager::import( const AttributeManager &attribute_manager,
absl::Span< const index_t > old2new,
const geode::uuid &attribute_id )
{
impl_->import( *attribute_manager.impl_, old2new, attribute_id,
AttributeBase::AttributeKey{} );
}

void AttributeManager::import( const AttributeManager &attribute_manager,
const GenericMapping< index_t > &old2new_mapping )
{
Expand Down
16 changes: 9 additions & 7 deletions src/geode/mesh/helpers/internal/copy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
*
*/

#include <geode/basic/mapping.hpp>

#include <geode/mesh/helpers/internal/copy.hpp>

#include <async++.h>
Expand All @@ -32,13 +34,13 @@ namespace geode
void copy_attributes(
const AttributeManager& manager_in, AttributeManager& manager_out )
{
absl::FixedArray< index_t > old2new( manager_in.nb_elements() );
async::parallel_for(
async::irange( index_t{ 0 }, manager_in.nb_elements() ),
[&old2new]( index_t i ) {
old2new[i] = i;
} );
manager_out.import( manager_in, old2new );
GenericMapping< index_t > mapping;
for( const auto attribute_element :
geode::Range{ manager_in.nb_elements() } )
{
mapping.map( attribute_element, attribute_element );
}
manager_out.import( manager_in, mapping );
}
} // namespace internal
} // namespace geode
14 changes: 7 additions & 7 deletions tests/basic/test-attribute.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,19 +637,18 @@ void test_import_manager( geode::AttributeManager& manager,
{
const auto nb_elements = manager.nb_elements();
geode::AttributeManager manager2;
std::vector< geode::index_t > old2new( nb_elements, geode::NO_ID );
geode::GenericMapping< geode::index_t > old2new_mapping;
manager2.resize( 0 );
manager2.import( manager, old2new );
manager2.import( manager, old2new_mapping );
test_attribute_types( manager2, bool_variable_attribute_id );
test_number_of_attributes( manager2, 8 );

geode::AttributeManager manager3;
for( const auto i : geode::LRange( nb_elements - 2 ) )
{
old2new[i] = nb_elements - 3 - i;
old2new_mapping.map( i, nb_elements - 3 - i );
}
manager3.resize( nb_elements - 2 );
manager3.import( manager, old2new );
manager3.import( manager, old2new_mapping );
test_attribute_types( manager3, bool_variable_attribute_id );
test_number_of_attributes( manager3, 8 );
auto array_attr =
Expand All @@ -663,12 +662,13 @@ void test_import_manager( geode::AttributeManager& manager,
"Error in attribute import value." );

geode::AttributeManager manager4;
old2new_mapping.clear();
for( const auto i : geode::LRange( nb_elements ) )
{
old2new[i] = i;
old2new_mapping.map( i, i );
}
manager4.resize( nb_elements );
manager4.import( manager, old2new );
manager4.import( manager, old2new_mapping );
test_attribute_types( manager4, bool_variable_attribute_id );
test_number_of_attributes( manager4, 8 );
test_sparse_attribute_after_element_deletion(
Expand Down
Loading