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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM with some nits! Totally awesome.



================
Comment at: llvm/include/llvm/IR/Instruction.h:59
+protected:
+  ~Instruction(); // Use deleteValue() to delete a generic Instruction.
 
----------------
rnk wrote:
> 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.
> ```
Neat!

It'd be awesome if there were a non-comment way of doing this, but hey... =]


================
Comment at: llvm/include/llvm/IR/Instruction.h:54-57
 public:
   Instruction(const Instruction &) = delete;
   Instruction &operator=(const Instruction &) = delete;
 
----------------
Sink this public region down to coalesce it with the one right below the protected detsructor?


================
Comment at: llvm/include/llvm/IR/User.h:96
   User(const User &) = delete;
-  ~User() override = default;
+protected:
+  ~User() = default;
----------------
rnk wrote:
> 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.
Maybe move the public constructor above down so that we don't have the 2 public regions split by a protected one?


================
Comment at: llvm/include/llvm/IR/Value.h:217
+
+public:
+  /// Delete a pointer to a generic Value.
----------------
Redundant `public:`.


https://reviews.llvm.org/D31261