Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Posted patch Remove nsTFixedString<T> (obsolete) — Splinter Review
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.
Attachment #8912630 - Flags: review?(michael)
Attachment #8912630 - Flags: review?(erahm)
(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 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.
Attachment #8912630 - Flags: review?(michael)
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.
Attachment #8912630 - Flags: review?(erahm) → review+
> 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?
Flags: needinfo?(michael)
(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);
    }
}
Flags: needinfo?(michael)
(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.
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.
Attachment #8914605 - Flags: review?(nika)
Attachment #8912630 - Attachment is obsolete: true
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?
Flags: needinfo?(dbaron)
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*.
Attachment #8914605 - Flags: review?(nika) → review+
(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?
Flags: needinfo?(dbaron)
Comment on attachment 8915015 [details]
Bug 1403506 - Remove nsTFixedString<T>. .

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

r=me
Attachment #8915015 - Flags: review?(erahm) → review+
Comment on attachment 8915015 [details]
Bug 1403506 - Remove nsTFixedString<T>. .

https://reviewboard.mozilla.org/r/186284/#review191486
Attachment #8915015 - Flags: review?(nika) → review+
https://hg.mozilla.org/mozilla-central/rev/15306340c1be
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.