Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.

LUCY-295 Adapt for changed return type of Vec_Get_Size#36

Merged
asfgit merged 4 commits intoapache:masterfrom
rectang:LUCY-295-vec-size
Apr 4, 2016
Merged

LUCY-295 Adapt for changed return type of Vec_Get_Size#36
asfgit merged 4 commits intoapache:masterfrom
rectang:LUCY-295-vec-size

Conversation

@rectang
Copy link
Contributor

@rectang rectang commented Mar 30, 2016

The Clownfish API Vec_Get_Size has changed from returning uint32_t to returning size_t. Adapt Lucy for the effect of this change.

This branch is broken up into commits according to the kind of remedy that was applied at each site, and the associated difficulty of proofing the change.

rectang added 4 commits March 29, 2016 21:04
Change the type of iteration variables to `size_t` to match the return
type of Vec_Get_Size.  This commit is restricted to simple cases where
the iteration variable is scoped to the loop block and is only used as
an C array index or arguments to Vector methods which take a `size_t`.
Widen the types of variables which are assigned the return value from
Vec_Get_Size.  The widening should have no impact on behavior or require
any casting; the variables should not be used in any context which
requires implicit conversion.
After converting variables assigned the return value of Vec_Get_Size
from `uint32_t` to `size_t`, at least one cast or other type conversion
is performed.
@nwellnhof
Copy link
Contributor

I see that you avoided any type changes to instance variables or method parameters and return values. So what we have now is a couple of explicit but still unchecked casts from size_t to uint32_t. This shouldn't matter in practice, but part of me prefers to convert all these variables and parameters to size_t, including I32Array and BitVector. Also, when serializing a size_t as 32-bit value, 100% correct code should check for overflow like

size_t size = Vec_Get_Size(vector);
if (size > UINT32_MAX) {
    THROW(...);
}
OutStream_Write_U32(outstream, (uint32_t)size);

(Or check before adding elements to such arrays.)

OTOH, this seems silly for things like the number of PolyQuery children, document fields, or sort rules.

@rectang
Copy link
Contributor Author

rectang commented Apr 4, 2016

I agree with making I32Array and BitVector use size_t internally to match the other types. Once they are converted, it will make sense to go back and fix the unchecked-but-explicit casts.

@asfgit asfgit merged commit 10c5775 into apache:master Apr 4, 2016
asfgit pushed a commit that referenced this pull request Apr 4, 2016
The Clownfish API `Vec_Get_Size` has changed from returning `uint32_t`
to returning `size_t`.  Adapt Lucy for the effect of this change.

This branch is broken up into commits according to the kind of remedy
that was applied at each site, and the associated difficulty of proofing
the change.

This closes #36.
@rectang rectang deleted the LUCY-295-vec-size branch April 4, 2016 23:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants