Page MenuHomePhabricator

[lldb] NFC remove DISALLOW_COPY_AND_ASSIGN
ClosedPublic

Authored by kwk on May 26 2020, 2:31 AM.

Details

Summary

This is how I applied my clang-tidy check (see
https://reviews.llvm.org/D80531) in order to remove
DISALLOW_COPY_AND_ASSIGN and have deleted copy ctors and deleted
assignment operators instead.

grep DISALLOW_COPY_AND_ASSIGN /opt/notnfs/kkleine/llvm/lldb -r -l | sort | uniq > files

for i in $(cat files);
do
  clang-tidy \
    --checks="-*,modernize-replace-disallow-copy-and-assign-macro" \
    --format-style=LLVM \
    --header-filter=.* \
    --fix \
    -fix-errors \
    $i;
done

Diff Detail

Event Timeline

kwk created this revision.May 26 2020, 2:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2020, 2:31 AM
kwk abandoned this revision.May 26 2020, 2:32 AM

Sorry, wrong revisions uploaded

kwk updated this revision to Diff 266136.May 26 2020, 2:33 AM

[lldb] Manual remove of DISALLOW_COPY_AND_ASSIGN def and one expansion

kwk edited the summary of this revision. (Show Details)May 26 2020, 2:35 AM
labath accepted this revision.May 26 2020, 3:50 AM
labath added a subscriber: labath.

I am all for this, because makes lldb code more consistent with llvm (llvm used to have a LLVM_DELETED_FUNCTION macro, but it was removed as soon as c++11 came into being).

But please don't commit this straight away -- let's wait a couple of days to give people a chance to comment on things.

This revision is now accepted and ready to land.May 26 2020, 3:50 AM
aprantl accepted this revision.May 26 2020, 11:15 AM
aprantl added a subscriber: aprantl.

I am all for this, because makes lldb code more consistent with llvm (llvm used to have a LLVM_DELETED_FUNCTION macro, but it was removed as soon as c++11 came into being).

That argument makes sense to me.

I am all for this, because makes lldb code more consistent with llvm (llvm used to have a LLVM_DELETED_FUNCTION macro, but it was removed as soon as c++11 came into being).

But please don't commit this straight away -- let's wait a couple of days to give people a chance to comment on things.

Wasn't LLVM_DELETED_FUNCTION not just a compatibility thing with certain MSVC versions that didn't handle = delete correctly? DISALLOW_COPY_AND_ASSIGN seems to be more about preventing copy-pasted declarations.

I'm not sure if I see the motivation for this change, but I also don't really see a problem if we remove this to get rid of another LLDB_* macro. So IMHO this is change is fine.

teemperor accepted this revision.May 26 2020, 11:21 AM

I am all for this, because makes lldb code more consistent with llvm (llvm used to have a LLVM_DELETED_FUNCTION macro, but it was removed as soon as c++11 came into being).

But please don't commit this straight away -- let's wait a couple of days to give people a chance to comment on things.

Wasn't LLVM_DELETED_FUNCTION not just a compatibility thing with certain MSVC versions that didn't handle = delete correctly?

Well, it was "compatibility" for the fact that MSVC did not support c++11, added back when c++11 was hot off the press. Before that llvm used /* DO NOT IMPLEMENT */ comments (that was way before my time, but that's what git history says).

DISALLOW_COPY_AND_ASSIGN seems to be more about preventing copy-pasted declarations.

Yes, that's kinda true. I was probably too harsh on it. I actually do see some appeal in that, but I don't think it's worth diverging from the llvm style because of it.

kwk retitled this revision from [lldb] Manual remove of DISALLOW_COPY_AND_ASSIGN def and one expansion to [lldb] NFC remove DISALLOW_COPY_AND_ASSIGN.May 28 2020, 12:56 PM
kwk edited the summary of this revision. (Show Details)
kwk updated this revision to Diff 267917.Jun 2 2020, 10:03 AM

Rebased

This revision was automatically updated to reflect the committed changes.