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

Reply via email to