[PATCH] D142214: [MC] Do not copy MCInstrDescs. NFC.

Fri Jan 20 11:00:06 PST 2023

foad added inline comments.

================
Comment at: llvm/include/llvm/MC/MCInstrDesc.h:200
 public:
+  MCInstrDesc(const MCInstrDesc &) = delete;
+
----------------
dblaikie wrote:
> foad wrote:
> > @dblaikie or any other C++ experts - does this seem reasonable? I was not sure if I should be following some kind of rule-of-3/5/7/11/13/... here. All I want to do is prevent users from //accidentally// writing `MCInstrDesc MCID = ...` instead of `const MCInstrDesc &MCID = ...`.
> Generally I'd suggest that APIs shouldn't make awkward ergonomic choices to reduce the chance of accidents - there's lots of things that can be expensive to copy in C++ that still have the usual copy ctor/assignment operator.
> 
> So if the thing is copyable - forcing users to do that copy in some uncommon way because the usual way is too error prone is, imho, not the right way to go - though I realize trying to teach people that this thing is expensive to copy isn't easy either. Hopefully updating a bunch of places that were copying will help set the right expectations.
> 
> If it's really non-copyable in any way, then, yes, the rule of 5 would apply. Implement the copy ctor, copy assignment, move ctor, move assignment, and destructor.
> 
> Sounds like deleting all the copy/move operations is what you want? Leave the dtor defaulted, maybe default the default (no-args) ctor? since that seems to be used in a test? (honestly, how are these objects constructed otherwise? I guess with aggregate init?)
Yes they are created with aggregate init, in a big static table generated by tablegen.

They are not expensive to copy. They "can't" be copied in that some methods would no longer work correctly, because they rely on being able to access other data in the same table at a known offset from "this".

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142214/new/

https://reviews.llvm.org/D142214