[PATCH] D60577: [X86AsmPrinter] refactor static functions into private methods. NFC

Eric Christopher via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 11 15:29:14 PDT 2019


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

In D60577#1463553 <https://reviews.llvm.org/D60577#1463553>, @smeenai wrote:

> In D60577#1463552 <https://reviews.llvm.org/D60577#1463552>, @srhines wrote:
>
> > In D60577#1463469 <https://reviews.llvm.org/D60577#1463469>, @smeenai wrote:
> >
> > > I have zero context on this change specifically, but "we should prefer functions in a private namespace" seems to be the exact opposite of the advice in https://llvm.org/docs/CodingStandards.html#static
> >
> >
> > While the commit comment references possibly moving things to a private namespace (which I agree is in contradiction to the referenced page), the actual code moves these to being private member functions. For functions that are taking `*this`, or propagating a `*this` in all usage, it seems fairly appropriate to me.  Updating the commit message to not disagree with the official guidance would be helpful.
>
>
> Yup, the change itself makes perfect sense, just that part of the commit message stood out :)


Agreed. That said, might be good to clean up the commit message with what you mean.

LGTM and thanks :)

-eric


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60577





More information about the llvm-commits mailing list