[PATCH] D140349: [llvm][PassSupport] don't require passes to be default constructible

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers planned changes to this revision.
nickdesaulniers marked 2 inline comments as done.
nickdesaulniers added inline comments.


================
Comment at: llvm/include/llvm/PassSupport.h:81-90
+template <class PassName,
+          typename std::enable_if<std::is_default_constructible<PassName>{},
+                                  bool>::type = true>
+Pass *callDefaultCtor() {
+  return new PassName();
+}
+
----------------
I can use C++14 std::enable_if_t to make this slightly more concise here.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.h:96
 
-  explicit AMDGPUDAGToDAGISel(TargetMachine *TM = nullptr,
-                              CodeGenOpt::Level OptLevel = CodeGenOpt::Default);
+  AMDGPUDAGToDAGISel() = delete;
+
----------------
arsenm wrote:
> nickdesaulniers wrote:
> > efriedma wrote:
> > > I don't think there's any point to explicitly deleting the default constructor (here and elsewhere); there's no implicitly-declared default constructor if there's an explicit constructor.
> > @arsenm from discord:
> > 
> > > really the default constructor should be deleted
> > 
> > @arsenm WDYT? (I don't care either way; but I'd prefer agreement between you and @efriedma on how I should proceed).
> I don't particularly care as long as there's no way to default construct. = delete just makes it clear that you shouldn't add it
I also prefer being explicit rather than try to remember the rule of 5 or 7 or gang of 4 or whatever. Marking this thread as done, but please re-comment/re-open @efriedma if you feel strongly otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140349