[PATCH] D87755: Silence GCC's `-Wclass-memaccess` warnings

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp:1719-1720
 
   InternalInstruction Insn;
-  memset(&Insn, 0, sizeof(InternalInstruction));
+  memset((void *)&Insn, 0, sizeof(InternalInstruction));
   Insn.bytes = Bytes;
----------------
jhenderson wrote:
> chandlerc wrote:
> > jhenderson wrote:
> > > This should work, right?
> > Not necessarily?
> > 
> > They have at least potentially different semantics (for example, if the default constructor would leave some members uninitialized for some reason). I'm not super familiar with this code and didn't really want to actually change its meaning, just make the compiler warning stop. ;]
> > 
> > If you or another are confident that the code doesn't need the memset, I can switch. Stylistically, I'd suggest:
> > ```
> > InternalInstruction Isns = {};
> > ```
> > 
> > But that's a minor point.
> Looking at the definition of `InternalInstruction`, I see usage of `ArrayRef`, various enum and integer types, bools and a pointer. There is no user-specified default constructor, so this should result in everything being value initialized, which with the potential exception of the `ArrayRef`, is the same as setting everything to zero in this context. Setting `ArrayRef` to zero using `memset` looks like it's the same as the `ArrayRef` default constructor too, so it should be safe, I believe.
> 
> NB: I am by no means an expert in this code area, so I might be missing something.
> 
> I'm more confident in the llvm-nm case, and the same logic applies - there is no default constructor, and the non-trivial member types all zero-initialize themselves when default constructed.
IMHO, we should respect the intent of that warning and not circumvent what constructors/assignment/copy constructors should be doing controlling. i.e. I prefer your suggestion of:

```
InternalInstruction Isns = {};
```

(and similar elsewhere)

As for preferring to keep this a NFC commit by silencing the warning with `(void *)` -- we have test coverage that should verify a semantic change is OK; we shouldn't treat these bits like [[ https://www.usenix.org/sites/default/files/conference/protected-files/srecon17americas_slides_reese.pdf | haunted graveyards ]].


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87755