fitzgen / inlinable_string

An owned, grow-able UTF-8 string that stores small strings inline and avoids heap-allocation.
http://fitzgen.github.io/inlinable_string/inlinable_string/index.html
Other
69 stars 11 forks source link

Implement some missing `std::string::String` methods #28

Open ImmemorConsultrixContrarie opened 4 years ago

ImmemorConsultrixContrarie commented 4 years ago

Closes #26

INLINE_STRING_CAPACITY: image image

No drain method; too much complicated code to write (even more complicated than it normally must be, because we are producing an iterator struct through trait there), while chars + remove_range pair does exactly the same job. Probably I need to write that in remove_range docs.

Minimum Rust version is 1.42 due to matches! macro in tests; I dunno if 1.41 would be minimum if user doesn't compile tests (some traits for String with 1.41+ orphan rules).

Crate major (middle, cuz it's pre-1.) version must be updated.

StringExt can no longer be a trait object.

ArtemGr commented 4 years ago

Hey! Thanks for the patch!

After a couple of months I finally have a little moment to look at what's going on here D

New methods are cool. If you would be so kind as to split it into smaller patches, one per new method perhaps, and start with the patches that retain the API and binary compatibility, I'm sure I'll find the time to check and then merge them.

As it is, I see some hard decisions here, like how much the usefulness of a trait object and maybe the added link time overhead due to more generics compare to the usefulness of new methods and the strain on developers from having an extra version to be aware of, and I would prefer to simply avoid making those (but of course maybe @fitzgen will want to make those decisions himself).

(I understant that the link time overhead here is likely neglible, but those add app and linking generics remains a bane of both C++ and Rust, I think. Libraries with simlper non-generic APIs are thus more welcome in my codebase; arguably generics also has a cognitive overhead for humans and not just compilers, cf. https://www.agilealliance.org/glossary/simple-design/ ?).

ImmemorConsultrixContrarie commented 4 years ago

Without RangeBounds there's no replace_range and remove_range. Each range object (.., x..y, etc.) is a unique type, thus the only way to do this without trait is sticking with some particular kind of range; this forbids other range types and that's not cool.

And retain can't be done without traits anyway, as there's no way to go with Fn but trait.

If you would be so kind as to split it into smaller patches

I'll try to do that.

ArtemGr commented 4 years ago

Without RangeBounds there's no replace_range and remove_range

Can't the generics live on another trait?

ArtemGr commented 4 years ago
   Compiling inlinable_string v0.1.11 (...)
error: unexpected closing delimiter: `}`
   --> src\inline_string.rs:505:5
    |
275 | impl InlineString {
    |                   - this opening brace...
...
504 |         }
    |         - ...matches this closing brace
505 |     }
    |     ^ unexpected closing delimiter

error: aborting due to previous error

error: could not compile `inlinable_string`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: unexpected closing delimiter: `}`
   --> src\inline_string.rs:505:5
    |
275 | impl InlineString {
    |                   - this opening brace...
...
504 |         }
    |         - ...matches this closing brace
505 |     }
    |     ^ unexpected closing delimiter

error: aborting due to previous error

error: build failed
ImmemorConsultrixContrarie commented 4 years ago

Can't the generics live on another trait?

They can. But then to get it all together you must either do StringExt + NonDynStringExt or trait StringMegaExt: StringExt + NonDynStringExt.

build failed

Yeah, that's bad, but gonna change with one of the next commits. I'm just doing git add -p on the changed files, grouping changes into commits that are easy to read and check.

ArtemGr commented 4 years ago

Right! But keep in mind that the larger the PR is the harder it is to review and accept D

ImmemorConsultrixContrarie commented 4 years ago

Right! But keep in mind that the larger the PR is the harder it is to review and accept D

Just do it on the per-commit basis, no?

ImmemorConsultrixContrarie commented 4 years ago

Okay, I found unsoundness in the insert_str PR, that you approved. I didn't look into it before, cuz I simply started adding commits on top of that PR, but now that I looked at it, I found it.

Plus, the test that I wrote and that must get this bug passes in debug mode due to assert_sanity function. Probably should remove that and instead write proper tests and // SAFETY: blocks for unsafe code.

ImmemorConsultrixContrarie commented 4 years ago

That's all what was written yet.