Skip to content

cleanup support - #268

Open
ChaoJia wants to merge 8 commits into
heremaps:masterfrom
ChaoJia:cleanup_support
Open

ChaoJia wants to merge 8 commits into
heremaps:masterfrom
ChaoJia:cleanup_support

Conversation

@ChaoJia

@ChaoJia ChaoJia commented Oct 29, 2026

Copy link
Copy Markdown

add cleanup support for flatdata.

Comment thread flatdata-rs/lib/src/filestorage.rs Resolved
Comment thread flatdata-rs/lib/src/storage.rs Resolved
Comment thread flatdata-rs/lib/src/storage.rs Resolved
TarArchiveResourceStorage::remove( const char* /* key */ )
{
// not supported since to TAR files is not supported
throw std::runtime_error( "Removing from TAR files is not supported" );

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we normally do not throw to report errors -> use error return paths?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Initially I had it return a bool in the c++ implementation, but later for the rust implementation I went with Result<(), io::Error>. which seems to me is essentially equivalent to exception in c++: if it's not handled anywhere along the call stack, it will lead to crash. Returned bool can easily get ignored. Should I return a bool or an error code for the c++ implementation?

/// remove resource and the corresponding schema
/// fail if resource or schema is missing or the existing schema doesn't match the expected
/// schema
#[doc(hidden)]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why hide it?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think this function is only supposed to be used in the generated clean_up functions. It doesn't seem to make sense to me to call this function explicitly on the consumer end, or is there a real use case for this?

}

/// remove multivector index resource
#[doc(hidden)]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here

@VeaaC

VeaaC commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator

You need to sign off the commits (see failed DCO checks)

Chao Jia and others added 7 commits November 11, 2026 14:56
Signed-off-by: Chao Jia <chao.jia@here.com>
Signed-off-by: Chao Jia <chao.jia@here.com>
Signed-off-by: Chao Jia <chao.jia@here.com>
Signed-off-by: Chao Jia <chao.jia@here.com>
Signed-off-by: Chao Jia <chao.jia@here.com>
Co-authored-by: epavese <epavese@gmail.com>
Signed-off-by: Chao Jia <chao.jia@here.com>
Signed-off-by: Chao Jia <chao.jia@here.com>
Signed-off-by: Chao Jia <chao.jia@here.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants