This is an archive of the discontinued LLVM Phabricator instance.

[libc++][NFC] remove this-> when calling member functions in <string>
ClosedPublic

Authored by philnik on Dec 27 2021, 6:00 PM.

Details

Summary

remove this-> when calling member functions

Diff Detail

Event Timeline

philnik requested review of this revision.Dec 27 2021, 6:00 PM
philnik created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2021, 6:00 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 396387.Dec 28 2021, 4:12 AM
  • Removed a few this-> I missed
Mordante accepted this revision as: Mordante.Dec 28 2021, 9:42 AM

I'm happy when the CI is happy, but the CI won't be happy ;-) D116334 should be a parent revision.

libcxx/include/string
1733

My suggestion here will merge-conflict with D116334, but: let's take this opportunity to be consistent. If we're removing this->s, we should also remove Base::s. Contra @Mordante below, I think CI will be happy with this patch, because while __basic_string_common<true> is a base class of a class template, it is not a dependent base class.

Btw, as a general rule I would much rather see this-> than Base::, when the thing is indeed a dependent base class. Saves brain cells and generally looks less arcane and scary.

However, in light of D116334 and my comment that I think you may be asked to keep the calls to the external library function for code-size reasons: maybe you should think about rewriting all these calls as __basic_string_common<true>::__throw_length_error(); so that then they'll keep working after __basic_string_common<true> becomes not-a-base-class-anymore.

Unless I'm mistaken, I think it will ultimately make sense to abandon D116324 and roll its surviving changes (if any) into D116334.

@Quuxplusone Any objections, since D116334 has been accepted by @ldionne?

Quuxplusone accepted this revision.Jan 24 2022, 11:20 AM

Nope, LGTM! (@Mordante already accepted, and I don't see anything weird here, so accepting as libc++)

This revision is now accepted and ready to land.Jan 24 2022, 11:20 AM
ldionne accepted this revision.Jan 24 2022, 12:16 PM

Parent revision means "needs to be shipped before this can be shipped", so I think D116334 is really a parent revision of this one, not a child, because you need to ship D116334 before you can ship this one (unless I missed something).

@ldionne I know. I based D116334 on this one and I'm planning to land them at the same time anyways. I just changed this to parent to make CI run properly. They should work independent of each other.

This revision was landed with ongoing or failed builds.Jan 24 2022, 3:23 PM
This revision was automatically updated to reflect the committed changes.