[PATCH] D31261: [IR] De-virtualize ~Value to save a vptr

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 11 inline comments as done.
rnk added inline comments.


================
Comment at: llvm/include/llvm/IR/Instruction.h:59
+protected:
+  ~Instruction(); // Use deleteValue() to delete a generic Instruction.
 
----------------
chandlerc wrote:
> Make this a doxygen comment?
The intention of these trailing comments is to make it so they show up in diagnostic notes when users try to delete a Value*. It looks like this:
```
C:\src\llvm-project\llvm\unittests\IR\InstructionsTest.cpp(410,10):  error: calling a protected destructor of class 'llvm::Instruction'
  delete I;
         ^
C:\src\llvm-project\llvm\include\llvm/IR/Instruction.h(59,3):  note: declared protected here
  ~Instruction(); // Use deleteValue() to delete a generic Instruction.
  ^
1 error generated.
```


================
Comment at: llvm/include/llvm/IR/User.h:96
   User(const User &) = delete;
-  ~User() override = default;
+protected:
+  ~User() = default;
----------------
chandlerc wrote:
> Add whitespace before the protected?
Done, I also added the trailing comment so it shows up in notes. People don't often delete a `User` directly, though.


================
Comment at: llvm/include/llvm/IR/Value.h:209-214
+  /// Value's destructor should be virtual by design, but that would require
+  /// that Value and all of its subclasses have a vtable that effectively
+  /// duplicates the information in the value ID. As a size optimization, the
+  /// destructor has been protected, and the caller should manually call
+  /// deleteValue.
+  ~Value(); // Use deleteValue() to delete a generic Value.
----------------
chandlerc wrote:
> Move this up to the protected region with the constructor?
> 
> Also, the trailing comment doesn't seem to add value at this point.
See example where it shows up in diagnostic notes but the doxygen one doesn't.


https://reviews.llvm.org/D31261