Clean up nsString implementation source files
Categories
(Core :: XPCOM, task)
Tracking
()
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.
Assignee | ||
Comment 1•2 years ago
|
||
This also changes the definition to be a static rather than a #define, in order
to reduce the potential impact.
Assignee | ||
Comment 2•2 years ago
|
||
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
Assignee | ||
Comment 3•2 years ago
|
||
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
Assignee | ||
Comment 4•2 years ago
|
||
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
Assignee | ||
Comment 5•2 years ago
|
||
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
Assignee | ||
Comment 6•2 years ago
|
||
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:
- 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). - 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. - 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. - The implementations were moved to nsTStringRepr.cpp.
- An overload of Find was added to try to catch callers which previously
calledFind(..., false)
orFind(..., true)
to set case-sensitivity, due
to booleans normally implicitly coercing toindex_type
. This should
probably be removed at some point, but may be useful during the transition.
Depends on D148299
Assignee | ||
Comment 7•2 years ago
|
||
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
Assignee | ||
Comment 8•2 years ago
|
||
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
Assignee | ||
Comment 9•2 years ago
|
||
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•2 years ago
|
||
Comment 11•2 years ago
|
||
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•2 years ago
|
||
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•2 years ago
|
||
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.
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 14•2 years ago
|
||
Comment 15•2 years ago
|
||
bugherder |
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
Assignee | ||
Updated•2 years ago
|
Description
•