1602452 - Several nsTLiteralString/nsTStringRepr member functions should be constexpr

Status: RESOLVED WONTFIX

(Core :: XPCOM, task)

Comment 1

[Simon Giesecke [:sg] [he/him]]

This is easy for the member functions defined in the header, it is just necessary to add constexpr.

It is also interesting for Equals and Compare, but the definitions of those functions need to be moved to the header as well. They could be implemented based on std::string_view, which already provides constexpr comparison operators.

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1602452 - Make member functions of nsTStringRepr defined in header constexpr. r=froydnj

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf906a905a26
Make member functions of nsTStringRepr defined in header constexpr. r=froydnj

Comment 4

[Alexandru Michis [:malexandru]]

Backed out changeset bf906a905a26 (Bug 1602452) for causing bustages in gtest/TestStrings.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/0df1383a748325b5957d1dc5427eb772d434f4f8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&collapsedPushes=598800&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=bf906a905a2635f08d055733627564a7f98b358b&tochange=0a4da12080809dc07c3c71cc27489f17348af7ec&searchStr=build&selectedJob=280297733

Failure logs:

• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280296180&repo=autoland&lineNumber=75626
• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280298089&repo=autoland&lineNumber=49799

Comment 5

[Simon Giesecke [:sg] [he/him]]

(In reply to Alexandru Michis [:malexandru] from comment #4)

Backed out changeset bf906a905a26 (Bug 1602452) for causing bustages in gtest/TestStrings.cpp

Windows build failures should be fixed by updated patch, running on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca71755198a7fa3bafe9b61c5aff4efecb74a1ec

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c6f4188a5bc
Make member functions of nsTStringRepr defined in header constexpr. r=froydnj

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1602452 - Make most remaining nsTStringRepr functions constexpr. r=froydnj

Comment 8

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1602452 - Make nsTSubstringTuple and operator+ returning nsTSubstringTuple constexpr. r=froydnj

Depends on D56682

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2c6f4188a5bc

Comment 10

[Simon Giesecke [:sg] [he/him]]

After abandoning revision https://bugzilla.mozilla.org/attachment.cgi?id=9115096:

My original idea was to allow to write something like:

constexpr auto a = NS_LITERAL_STRING("a");
constexpr auto b = NS_LITERAL_STRING("b");
constexpr auto ab = a + b;

This would frequently be helpful. However, this is not possible since nsTSubstringTuple is a MOZ_TEMPORARY_CLASS. Alternatively, another overload of operator+ accepting only nsTLiteralString (or perhaps more explicitly a named function, e.g. ConcatConstexpr(...)) that allows to do this, could be defined, which doesn't return nsTSubstringTuple but a new class, which does not need to be MOZ_TEMPORARY_CLASS and might be based on an array rather than a binary tree. (I have done this in the past in another codebase.) What do you think about that?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #10)

After abandoning revision https://bugzilla.mozilla.org/attachment.cgi?id=9115096:

My original idea was to allow to write something like:

constexpr auto a = NS_LITERAL_STRING("a");
constexpr auto b = NS_LITERAL_STRING("b");
constexpr auto ab = a + b;

This would frequently be helpful. However, this is not possible since nsTSubstringTuple is a MOZ_TEMPORARY_CLASS. Alternatively, another overload of operator+ accepting only nsTLiteralString (or perhaps more explicitly a named function, e.g. ConcatConstexpr(...)) that allows to do this, could be defined, which doesn't return nsTSubstringTuple but a new class, which does not need to be MOZ_TEMPORARY_CLASS and might be based on an array rather than a binary tree. (I have done this in the past in another codebase.) What do you think about that?

I am ambivalent on this, but it's not really my decision anymore. :)

Comment 12

[Simon Giesecke [:sg] [he/him]]

Nika, I think we discussed the question from Comment 10 some time ago and I think you also said this wouldn't be widely useful (the parts of the codebase that use SQL queries like dom/indexedDB might be special here). The question remains if at least some of the constexpr'ing changes of the attached patch (https://phabricator.services.mozilla.com/D56525) are still useful.

Comment 13

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

I don't think they're necessary right now, so we can probably hold off on the patch for now. Thanks

Comment 14

[BugBot [:suhaib / :marco]]

The leave-open keyword is there and there is no activity for 6 months.
:nika, maybe it's time to close this bug?

Comment 15

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

Closing as I don't think we intend to fix this in the near-term.