1403506 - Remove nsTFixedString<T>

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

nsTFixedString<T> is only used as a base class for nsTAutoStringN<T, N>, so
this patch merges the former into the latter, cutting some code and simplifying
the string class hierarchy.

Because the "Fixed" name is now gone, the patch also renames
StringDataFlags::FIXED as INLINE and ClassDataFlags::FIXED as AUTO.

The patch also removes nsFixed[C]String and ns_auto_[c]string! from Rust code
because nsAutoString can't be implemented directly in Rust due to its move
semantics. There were only two uses of ns_auto_string! outside of tests so this
seems like a minor loss.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove nsTFixedString<T>

erahm, please review the C++ changes.

mystor, please review the Rust changes. Note that I modified both
servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs and
xpcom/rust/nsstring/src/lib.rs; these two files have a lot of overlap and I'm
not entirely sure about their relationship.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> mystor, please review the Rust changes. Note that I modified both
> servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs and
> xpcom/rust/nsstring/src/lib.rs; these two files have a lot of overlap and I'm
> not entirely sure about their relationship.

Awesome, thanks for doing that. Currently the situation is that these two files have to be kept in sync (although we've failed to do that in the past). We don't want to have to do that anymore, so we're probably going to change that in bug 1403213, which will mean the tree has one canonical version, and stylo gets its copy from crates.io when it needs to build outside of m-c.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8912630 [details] [diff] [review]
Remove nsTFixedString<T>

Review of attachment 8912630 [details] [diff] [review]:
-----------------------------------------------------------------

Yesterday I landed some changes to the rust nsstring bindings (bringing nsstring_vendor more in line with nsstring), so I'd like to see this again once this is rebased on top of that. I'm only reviewing the changes in the xpcom/rust version right now.

The rust changes look fine, although it makes me sad that we are losing the ability to use cheaper stack-allocated string buffers from rust code, and instead always have to allocate. Perhaps we'll develop better solutions in the future as they become important.

::: xpcom/rust/nsstring/gtest/Test.cpp
@@ +32,5 @@
>        static void RunTest() {                                           \
>          size_t size, align, offset;                                     \
>          Rust_Test_Member_##Clazz##_##Member(&size, &align, &offset);    \
>          EXPECT_EQ(size, sizeof(mozilla::DeclVal<Hack>().Member));       \
> +        EXPECT_EQ(align, alignof(decltype(mozilla::DeclVal<Hack>().Member))); \

Oops, that's embarrassing, thanks for catching that.

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8912630 [details] [diff] [review]
Remove nsTFixedString<T>

Review of attachment 8912630 [details] [diff] [review]:
-----------------------------------------------------------------

Nice cleanup, I'd defer to dbaron on the matter of renaming fixed -> inline but he's not accepting feedback requests. I'd either land this as-is or maybe split the inline stuff out to a separate follow up patch.

Comment 5

[Nicholas Nethercote [inactive]]

> The rust changes look fine, although it makes me sad that we are losing the ability to use
> cheaper stack-allocated string buffers from rust code, and instead always have to allocate.
> Perhaps we'll develop better solutions in the future as they become important.

There is a way to retain this ability: just keep the implementations of nsFixed[C]String and ns_auto_[c]string! on the Rust side. Though we'd probably want to rename nsFixed[C]String to nsAuto[C]StringHelper or something like that, to indicate that it's not actually reflecting a gecko type, but just needed for ns_auto_[c]string. The only downside is that I don't get to remove nearly as much code :)

mystor, what do you think?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> There is a way to retain this ability: just keep the implementations of
> nsFixed[C]String and ns_auto_[c]string! on the Rust side. Though we'd
> probably want to rename nsFixed[C]String to nsAuto[C]StringHelper or
> something like that, to indicate that it's not actually reflecting a gecko
> type, but just needed for ns_auto_[c]string. The only downside is that I
> don't get to remove nearly as much code :)
> 
> mystor, what do you think?

I actually think that we can get the best of both worlds, both removing nsFixed[C]String completely, and keeping the ns_auto_[c]string! macros. If we pull some trickery with the macros, we can prevent people from moving the value which we allocate by only giving them an &mut nsA[C]String reference, allowing us to use nsAuto[C]String-style inline storage directly within the type, without the need for nsFixed[C]Strings.

It'd probably look something like this (untested):

#[macro_export]
macro_rules! ns_auto_cstring {
    ($name:ident, $N:expr) => {
        // Create a struct on the stack with an identical layout to an nsAutoCString.
        // Hygiene makes it so callers can't touch internal, which would be unsafe.
        let mut internal = unsafe {
            const INLINE_CAPACITY: usize = $N;
            #[repr(C)]
            struct AutoString {
                _base: $crate::nsCStringRepr,
                inline_capacity: usize,
                inline_storage: [0; INLINE_CAPACITY],
            }
            AutoString {
                // NOTE: nsCStringRepr::new would need to be made public & unsafe.
                _base: $crate::nsCStringRepr::new(
                    $crate::class_flags::AUTO | $crate::class_flags::NULL_TERMINATED
                ),
                inline_capacity: INLINE_CAPACITY,
                inline_storage: [0; INLINE_CAPACITY],
            }
        };

        // Expose a mutable reference to it as &mut nsACString.
        let $name: &mut nsACString = &mut internal._base;
    };
    ($name:ident) => {
        ns_auto_cstring!($name, 64);
    }
}

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(In reply to Michael Layzell [:mystor] from comment #6)
> It'd probably look something like this (untested):

Oops, I forgot the destructor.

AutoString will need a Drop implementation as well.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Remove nsTFixedString<T>

Nika: I've rebased over the nsstring bindings changes, as requested.

Your suggestion in comment 6 is clever and I considered doing it, but in the
end I figured it wasn't worth the extra complexity for just two cases. I did
add a comment in the code referring to that Bugzilla comment, in case the need
for such types increases in the future.

Comment 9

[Nicholas Nethercote [inactive]]

dbaron: in this bug I'm removing nsFixed[C]String my merging them into nsAuto[C]String, because that's the only place they are used. As part of that I've renamed the following constants:

- DataClassFlags::FIXED --> INLINE, because that describes the nature of the char storage.

- StringClassFlags::FIXED --> AUTO, because that matches the nsAuto[C]String class name.

Sound ok to you?

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8914605 [details] [diff] [review]
Remove nsTFixedString<T>

Review of attachment 8914605 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks :-).

I agree that we probably don't need it now, and if we do need it in the future, we can just pull the macro trick *shrug*.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> dbaron: in this bug I'm removing nsFixed[C]String my merging them into
> nsAuto[C]String, because that's the only place they are used. As part of
> that I've renamed the following constants:
> 
> - DataClassFlags::FIXED --> INLINE, because that describes the nature of the
> char storage.
> 
> - StringClassFlags::FIXED --> AUTO, because that matches the nsAuto[C]String
> class name.
> 
> Sound ok to you?

Mostly, although I guess I'd somewhat prefer if the names were more similar.  Maybe the StringClassFlags one could be INLINE_STORAGE, even though that doesn't match the name?

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Bug 1403506 - Remove nsTFixedString<T>. .

(patch is actually r=erahm,mystor)

nsTFixedString<T> is only used as a base class for nsTAutoStringN<T, N>, so
this patch merges the former into the latter, cutting some code and simplifying
the string class hierarchy.

Because the "Fixed" name is now gone, the patch also renames
StringDataFlags::FIXED as INLINE and ClassDataFlags::FIXED as INLINE.

The patch also removes nsFixed[C]String and ns_auto_[c]string! from Rust code
because nsAutoString can't be implemented directly in Rust due to its move
semantics. There were only two uses of ns_auto_string! outside of tests so this
seems like a minor loss.

Review commit: https://reviewboard.mozilla.org/r/186284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186284/

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8915015 [details]
Bug 1403506 - Remove nsTFixedString<T>. .

https://reviewboard.mozilla.org/r/186284/#review191302

r=me

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15306340c1be
Remove nsTFixedString<T>. r=erahm.

Comment 15

[Nicholas Nethercote [inactive]]

https://github.com/servo/servo/pull/18736 is the Servo PR.

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8915015 [details]
Bug 1403506 - Remove nsTFixedString<T>. .

https://reviewboard.mozilla.org/r/186284/#review191486

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/15306340c1be