aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM, thank you! ================ Comment at: clang-tidy/modernize/UseEqualsDeleteCheck.cpp:58 + << FixItHint::CreateInsertion(StartLoc, "public: ") + << FixItHint::CreateInsertion(AfterLoc, " private:"); +} ---------------- malcolm.parsons wrote: > 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. I think that's a good solution for now, thanks! https://reviews.llvm.org/D26138 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
