This is an archive of the discontinued LLVM Phabricator instance.

[APInt] Allow self-assignment with libstdc++
ClosedPublic

Authored by vitalybuka on Aug 17 2020, 3:01 AM.

Diff Detail

Event Timeline

vitalybuka created this revision.Aug 17 2020, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 3:01 AM
vitalybuka requested review of this revision.Aug 17 2020, 3:02 AM
craig.topper added inline comments.Aug 18 2020, 4:25 PM
llvm/unittests/ADT/APIntTest.cpp
1785–1786

This comment needs to be updated

I would prefer to remove if def completly and always compile with if (this == &that)
Original patch justified removing of (this == &that) to save 4k. I don't think it's worth it.
However if want to remove this from release maybe we should do the following, as EXPENSIVE_CHECKS is the one who triggers std::shuffle in llvm::sort?

#ifdef EXPENSIVE_CHECKS
   if (this == &that)
      return *this;
#endif

update comment

vitalybuka marked an inline comment as done.Aug 18 2020, 4:31 PM

I would prefer to remove if def completly and always compile with if (this == &that)
Original patch justified removing of (this == &that) to save 4k. I don't think it's worth it.
However if want to remove this from release maybe we should do the following, as EXPENSIVE_CHECKS is the one who triggers std::shuffle in llvm::sort?

#ifdef EXPENSIVE_CHECKS
   if (this == &that)
      return *this;
#endif

This makes sense to me - we keep the codesize savings on main builds and we have a better chance of seeing self-moves on windows bots.

Use EXPENSIVE_CHECKS

RKSimon accepted this revision.Aug 20 2020, 4:09 AM

LGTM

This revision is now accepted and ready to land.Aug 20 2020, 4:09 AM
This revision was landed with ongoing or failed builds.Aug 20 2020, 4:14 AM
This revision was automatically updated to reflect the committed changes.