Closed Bug 1772006 Opened 3 years ago Closed 2 years ago

Clean up nsString implementation source files

Categories

(Core :: XPCOM, task)

task

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(8 files, 1 obsolete file)

(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details

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.

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

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

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

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

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

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

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

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

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

Pushed by nlayzell@mozilla.com: 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

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

Flags: needinfo?(nika)

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.

Attachment #9279647 - Attachment is obsolete: true

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.

Flags: needinfo?(nika)
Flags: needinfo?(brennie)
Blocks: 1778429
Flags: needinfo?(brennie)
Attachment #9279648 - Attachment description: Bug 1772006 - Part 2: Split nsSubstring.cpp into relevant files, r=#xpcom-reviewers → Bug 1772006 - Part 1: Split nsSubstring.cpp into relevant files, r=#xpcom-reviewers
Attachment #9279649 - Attachment description: Bug 1772006 - Part 3: Support converting nsTStringRepr<T> to std::basic_string_view<T>, r=#xpcom-reviewers → Bug 1772006 - Part 2: Support converting nsTStringRepr<T> to std::basic_string_view<T>, r=#xpcom-reviewers
Attachment #9279650 - Attachment description: Bug 1772006 - Part 4: Move basic string APIs to more appropriate .cpp files, r=#xpcom-reviewers → Bug 1772006 - Part 3: Move basic string APIs to more appropriate .cpp files, r=#xpcom-reviewers
Attachment #9279651 - Attachment description: Bug 1772006 - Part 5: Simplify and move the string comparison APIs from ns[T]StringObsolete, r=#xpcom-reviewers → Bug 1772006 - Part 4: Simplify and move the string comparison APIs from ns[T]StringObsolete, r=#xpcom-reviewers
Attachment #9279652 - Attachment description: Bug 1772006 - Part 6: Simplify and move the string searching APIs from ns[T]StringObsolete, r=#xpcom-reviewers → Bug 1772006 - Part 5: Simplify and move the string searching APIs from ns[T]StringObsolete, r=#xpcom-reviewers
Attachment #9279653 - Attachment description: Bug 1772006 - Part 7: Simplify and move the find+replace methods from ns[T]StringObsolete, r=#xpcom-reviewers → Bug 1772006 - Part 6: Simplify and move the find+replace methods from ns[T]StringObsolete, r=#xpcom-reviewers
Attachment #9279654 - Attachment description: Bug 1772006 - Part 8: Remove unnecessary files from the string implementation, r=#xpcom-reviewers → Bug 1772006 - Part 7: Remove unnecessary files from the string implementation, r=#xpcom-reviewers
Attachment #9279655 - Attachment description: Bug 1772006 - Part 9: Allow building xpcom/string without unified builds, r=#xpcom-reviewers → Bug 1772006 - Part 8: Allow building xpcom/string without unified builds, r=#xpcom-reviewers
No longer blocks: 1778429
Pushed by nlayzell@mozilla.com: 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
Blocks: 1782347
Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: