malcolm.parsons planned changes to this revision. malcolm.parsons added inline comments.
================ Comment at: clang-tidy/modernize/UseEqualsDeleteCheck.cpp:58 + << FixItHint::CreateInsertion(StartLoc, "public: ") + << FixItHint::CreateInsertion(AfterLoc, " private:"); +} ---------------- aaron.ballman wrote: > malcolm.parsons wrote: > > aaron.ballman wrote: > > > I am on the fence about this fixit. On the one hand, the fix is a > > > technical improvement because it means that implementations will > > > consistently find the declaration and bark about it being explicitly > > > deleted. On the other hand, the fixit suggests code that should never > > > pass a reasonable code review. > > > > > > I'm wondering if it would make more sense to leave the access specifiers > > > alone and just put a FIXME in the code to point the situation out. I am > > > guessing that at some point we will have a refactoring tool that can help > > > without resorting to making declarations like `public: C() = delete; > > > private:`. > > > > > > What do you think? > > I'm wondering whether no fixit would be better than a not-good-enough fixit. > Generally, no. Incremental improvements are almost always fine. However, the > user is asking to have their code modernized, and the fixit results in code > that looks more foreign than modern (at least, to me). > > I won't block the patch moving forward regardless of whether the fixit is in > or out, but I am curious if @alexfh has an opinion, or if you have a strong > preference one way or the other. I'll change the check to warn about deleted methods that aren't public; the user can fix those manually until a better fixit is possible. https://reviews.llvm.org/D26138 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
