Repository navigation
Added Extend and various FromIterator implementations to CslStringList. - #457
Conversation
|
@lnicola @jdroenner r+? |
|
Sorry, I was away for the past couple of days. I'll take a look tomorrow. |
| @@ -1,3 +1,12 @@ | |||
| <PAMDataset> | |||
| <SRS dataAxisToSRSAxisMapping="2,1">GEOGCS["WGS 84",DATUM["WGS_1984",SPHEROID["WGS 84",6378137,298.257223563,AUTHORITY["EPSG","7030"]],AUTHORITY["EPSG","6326"]],PRIMEM["Greenwich",0,AUTHORITY["EPSG","8901"]],UNIT["degree",0.0174532925199433,AUTHORITY["EPSG","9122"]],AXIS["Latitude",NORTH],AXIS["Longitude",EAST],AUTHORITY["EPSG","4326"]]</SRS> | |||
| <PAMRasterBand band="1"> | |||
There was a problem hiding this comment.
I don't remember if we want this or not, but perhaps it's harmless.
There was a problem hiding this comment.
it doesn't affect the tests. I got tired of reseting the file every time I ran tests.
| } | ||
| } | ||
|
|
||
| impl Extend<CslStringListEntry> for CslStringList { |
There was a problem hiding this comment.
Too bad we can't use CSLMerge here.
There was a problem hiding this comment.
I actually started to implement it (and still can) but the C++ implementation scared me as I saw no evidence that it would extend the length of the original array. That said, my C++ is 20 years rusty.
There was a problem hiding this comment.
That function just appends one element at a time.
There was a problem hiding this comment.
Couldn't find a {re|m}alloc in there or in CSLSetNameValue either.
There was a problem hiding this comment.
| impl Extend<CslStringListEntry> for CslStringList { | ||
| fn extend<T: IntoIterator<Item = CslStringListEntry>>(&mut self, iter: T) { | ||
| for e in iter { | ||
| self.add_entry(&e).unwrap_or_default(); |
There was a problem hiding this comment.
@lnicola Here's my rational as to why this might be OK.
Most of the time you're going to be extending one CslStringList with another. The only way that call can fail is if your insertion String has \r, \n, or most importantly \0. If iter comes from a CslStringList then that validation has already happened.
However, if iter comes from [CslStringListEntry], then you could presumably circumvent that check. I added this comment for that reason.
The obvious alternative means no Extend, and TryFroms to cover what is probably a rare edge case. On balance, I'd rather have Extend.
There was a problem hiding this comment.
Exactly. OOM might be another case where it fails, but it's pretty unlikely.
There was a problem hiding this comment.
FWIW, Added additional documentation to CslStringListEntry::new_{pair|flag}.
lnicola
left a comment
lnicola
left a comment
There was a problem hiding this comment.
The error handling makes this a bit icky, but I don't have any good suggestions.
I think it's workable, if not ideal, let's see how jdroenner, feels about it.
|
@jdroenner r+? |
| /// | ||
| /// An Entry is ether a single token (`Flag`), or a `name=value` assignment (`Pair`). | ||
| /// | ||
| /// Note: When constructed directly, assumes string values do not contain newline characters nor |
There was a problem hiding this comment.
We should probably spell out more clearly that those values will be ignored. And not just here, but also on CslStringList.
There was a problem hiding this comment.
CslStringList actually propagates the error.
Lines 116 to 117 in 16f2aef
to `CslStringList`.
caa66a6 to
ff6a0e3
Compare
|
bors r=lnicola |
|
bors try |
2 participants
CHANGES.mdif knowledge of this change could be valuable to users.Addresses some leftover feedback from here.
The docs summarize what this PR enables: