chore: Remove Status from ArraySchema, Attribute, Dimension, Domain #5541
Conversation
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
bekadavis9
reviewed
Jun 12, 2025
| @@ -515,25 +512,25 @@ class ArraySchema { | |||
| * Sets whether the array allows coordinate duplicates. | |||
| * It errors out if set to `1` for dense arrays. | |||
| */ | |||
| Status set_allows_dups(bool allows_dups); | |||
| void set_allows_dups(bool allows_dups); | |||
There was a problem hiding this comment.
Same as above. Should return bool. I won't continue to mark the rest of these for both of our sake :')
There was a problem hiding this comment.
Only has_dimension and has_attribute have value which can now be return this way, the other affected functions do not have out arguments.
| if (domain == nullptr) { | ||
| return; | ||
| } | ||
| throw DimensionException( | ||
| std::string("Setting the domain to a dimension with type '") + |
There was a problem hiding this comment.
Nit: no need to cast the exception body to std::string. Rings true throughout PR.
There was a problem hiding this comment.
This is needed for the concatenation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There are a number of old stories which refer to "un-
Statusing" various parts of the code. I can imagine some benefits of doing this, but a new one concerns our upcoming interoperability with Rust code.The Rust crate we use to interoperate with C++,
cxx, can generate idiomatic bindings in Rust to use types and call functions which are defined in C++. These bindings can declare and use "opaque" types, i.e. types whose definitions reside within C++ and are invisible to Rust code. However, these types cannot be passed by value to and from Rust functions.Statusis commonly used as a return value from our internal C++ code. This prevents us from declaring bindings for those functions incxxand thus calling them from Rust. We probably cannot migrateStatusfrom an opaque type to a shared type, but we can "un-Status" the functions we want to use.This pull request "un-
Statuses" the methods of classes in thearray_schemadirectory. This will eventually allow them to be called from Rust code.TYPE: NO_HISTORY
DESC: Un-Status array_schema methods