1772006 - Clean up nsString implementation source files

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

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

Currently the implementations for ns[C]String and related types are spread out inconsistently over many source files in a confusing manner, due to how it was implemented before the changes in bug 1393230. This ends up making the code dependent on unified builds, and also makes it more complicated to work with than before.

This bug will track simplifying the code significantly such that each type is implemented in a single file, much of the duplicated or legacy logic in ns[T]StringObsolete.cpp is removed, and the directory can successfully build with unified builds disabled.

Comment 1

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

Attachment: Bug 1772006 - Part 1: Move definition of `kNotFound` to `nsStringFwd.h`, r=#xpcom-reviewers

This also changes the definition to be a static rather than a #define, in order
to reduce the potential impact.

Comment 2

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

Attachment: Bug 1772006 - Part 1: Split nsSubstring.cpp into relevant files, r=#xpcom-reviewers

This file only exists as a wrapper around nsTSubstring.cpp, with some methods
left in it due to how the string types were defined before they were turned
into templates.

Depends on D148295

Comment 3

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

Attachment: Bug 1772006 - Part 2: Support converting nsTStringRepr<T> to std::basic_string_view<T>, r=#xpcom-reviewers

This type was introduced in c++17, and can be used as a convenient standard
medium for passing around borrowed substring references. It can be implicitly
converted to from string literals and const char_type*, meaning that after
this change it can be used as a convenient catch-all type to replace seperate
overloads for const self_type&, const char_type* and const char_type(&)[N].

std::basic_string_view also provides standard implementations of some
algorithms which will be convenient for code cleanup in later parts of this
bug.

Depends on D148296

Comment 4

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

Attachment: Bug 1772006 - Part 3: Move basic string APIs to more appropriate .cpp files, r=#xpcom-reviewers

Due to the history of how nsString was implemented, many APIs ended up being
implemented in inappropriate files. This change moves simple definitions which
won't require any changes for each type to happen within that type's .cpp file.

This is necessary to eventually enable the directory to be built without
unified builds.

Depends on D148297

Comment 5

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

Attachment: Bug 1772006 - Part 4: Simplify and move the string comparison APIs from ns[T]StringObsolete, r=#xpcom-reviewers

In addition to moving these methods to a more appropriate file, they were
simplified to make them easier to maintain in the future.
nsTStringRepr::Compare was extended to also work on char16_t strings, and the
case insensitive and other options were removed as they aren't necessary. This
required some changes to callers in the tree.

The EqualsIgnoreCase method was also simplified by using std::string_view.

Depends on D148298

Comment 6

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

Attachment: Bug 1772006 - Part 5: Simplify and move the string searching APIs from ns[T]StringObsolete, r=#xpcom-reviewers

The biggest set of APIs from ns[T]StringObsolete which are still heavily used
are the string searching APIs. It appears the intention was for these to be
replaced by the FindInReadable APIs, however that doesn't appear to have
happened.

In addition, the APIs have some quirks around their handling of mixed character
widths. These APIs generally supported both narrow strings and the native
string type, probably because char16_t string literals weren't available until
c++11. Finally they also used easy-to-confuse unlabeled boolean and integer
optional arguments to control behaviour.

These patches do the following major changes to the searching APIs:

1. The ASCII case-insensitive search method was split out as
   LowerCaseFindASCII, rather than using a boolean. This should be less
   error-prone and more explicit, and allows the method to continue to use
   narrow string literals for all string types (as only ASCII is supported).
2. The other [R]Find methods were restricted to only support arguments with
   matching character types. I considered adding a FindASCII method which would
   use narrow string literals for both wide and narrow strings but it would've
   been the same amount of work as changing all of the literals to unicode
   literals.
   This ends up being the bulk of the changes in the patch.
3. All find methods were re-implemented using std::basic_string_view's find
   algorithm or stl algorithms to reduce code complexity, and avoid the need to
   carry around the logic from nsStringObsolete.cpp.
4. The implementations were moved to nsTStringRepr.cpp.
5. An overload of Find was added to try to catch callers which previously
   called Find(..., false) or Find(..., true) to set case-sensitivity, due
   to booleans normally implicitly coercing to index_type. This should
   probably be removed at some point, but may be useful during the transition.

Depends on D148299

Comment 7

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

Attachment: Bug 1772006 - Part 6: Simplify and move the find+replace methods from ns[T]StringObsolete, r=#xpcom-reviewers

The remaining methods in ns[T]StringObsolete are all find+replace methods for
nsTSubstring. These were migrated in a similar way to the find methods, and
partially updated to avoid using methods from nsStringObsolete.cpp.

This change removes the ns[T]StringObsolete.cpp files completely, as they are
no longer necessary.

Depends on D148300

Comment 8

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

Attachment: Bug 1772006 - Part 7: Remove unnecessary files from the string implementation, r=#xpcom-reviewers

These files exist only due to the history of how strings were previously
declared using macros, and aren't necessary anymore. The header files were not
removed, as they may be depended on from outside of the module. Cleaning up the
header situation is something which should probably happen in a follow-up.

Depends on D148301

Comment 9

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

Attachment: Bug 1772006 - Part 8: Allow building xpcom/string without unified builds, r=#xpcom-reviewers

The last remaining things requiring unified builds in this directory are the
explicit specializations. As each class' methods are now confined to a single
file, these can now be moved to the appropriate .cpp files.

Depends on D148302

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/962bfea4a309
Part 1: Move definition of `kNotFound` to `nsStringFwd.h`, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/b2542aef3054
Part 2: Split nsSubstring.cpp into relevant files, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/42730aae16ea
Part 3: Support converting nsTStringRepr<T> to std::basic_string_view<T>, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/79a734b4e4d9
Part 4: Move basic string APIs to more appropriate .cpp files, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/790f42b64af9
Part 5: Simplify and move the string comparison APIs from ns[T]StringObsolete, r=xpcom-reviewers,necko-reviewers,dragana,barret
https://hg.mozilla.org/integration/autoland/rev/b6611ab002d9
Part 6: Simplify and move the string searching APIs from ns[T]StringObsolete, r=xpcom-reviewers,necko-reviewers,eeejay,dragana,barret
https://hg.mozilla.org/integration/autoland/rev/d19663161261
Part 7: Simplify and move the find+replace methods from ns[T]StringObsolete, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/c725fe1f5882
Part 8: Remove unnecessary files from the string implementation, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/f17c7565707b
Part 9: Allow building xpcom/string without unified builds, r=xpcom-reviewers,barret

Comment 11

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing build bustages on nsTString.cpp

Backout link
Push with failures
Link to failure log
Failure line :
/builds/worker/checkouts/gecko/xpcom/string/nsTString.cpp:57:3: error: unknown type name 'NS_LossyConvertUTF16toASCII'

Link to failure log 2
Failure line :
TEST-UNEXPECTED-TIMEOUT | netwerk/test/unit/test_http2.js | Test timed out

Link to failure log 3
Failure line :
TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/mochitest/browser_save_filenames.js | Test timed out

Link to failure log 4
Failure line :
TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/mochitest/browser_save_filenames.js | file i66 index_002 was saved with the correct name using saveDocument

Comment 12

[Phabricator Automation]

Comment on attachment 9279647 [details]
Bug 1772006 - Part 1: Move definition of kNotFound to nsStringFwd.h, r=#xpcom-reviewers

Revision D148295 was moved to bug 1773604. Setting attachment 9279647 [details] to obsolete.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nika, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f6e8ae5a4b1
Part 1: Split nsSubstring.cpp into relevant files, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/f472530da56f
Part 2: Support converting nsTStringRepr<T> to std::basic_string_view<T>, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/05e1d69d0d07
Part 3: Move basic string APIs to more appropriate .cpp files, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/8235755979d7
Part 4: Simplify and move the string comparison APIs from ns[T]StringObsolete, r=xpcom-reviewers,necko-reviewers,dragana,barret
https://hg.mozilla.org/integration/autoland/rev/e808707f7d80
Part 5: Simplify and move the string searching APIs from ns[T]StringObsolete, r=xpcom-reviewers,necko-reviewers,eeejay,dragana,barret
https://hg.mozilla.org/integration/autoland/rev/f1afa93d7711
Part 6: Simplify and move the find+replace methods from ns[T]StringObsolete, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/3db13c3a5097
Part 7: Remove unnecessary files from the string implementation, r=xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/79cc3e7c7045
Part 8: Allow building xpcom/string without unified builds, r=xpcom-reviewers,barret

Comment 15

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/4f6e8ae5a4b1
https://hg.mozilla.org/mozilla-central/rev/f472530da56f
https://hg.mozilla.org/mozilla-central/rev/05e1d69d0d07
https://hg.mozilla.org/mozilla-central/rev/8235755979d7
https://hg.mozilla.org/mozilla-central/rev/e808707f7d80
https://hg.mozilla.org/mozilla-central/rev/f1afa93d7711
https://hg.mozilla.org/mozilla-central/rev/3db13c3a5097
https://hg.mozilla.org/mozilla-central/rev/79cc3e7c7045