Page MenuHomePhabricator

[X86AsmPrinter] refactor static functions into private methods. NFC
ClosedPublic

Authored by nickdesaulniers on Apr 11 2019, 1:39 PM.

Details

Summary

A lot of the code for printing special cases of operands in this
translation unit are static functions. While I too have suffered many
years of abuse at the hands of C, we should prefer private methods,
particularly when you start passing around *this as your first argument,
which is a code smell.

This will help make generic vs arch specific asm printing easier, as it
brings X86AsmPrinter more in line with other arch's derived AsmPrinters.
We will then be able to more easily move architecture generic code to
the base class, and architecture specific code to the derived classes.

Some other small refactorings while we're here:

  • the parameter Op is now consistently OpNo
  • add spaces around binary expressions. I know we're not millionaires but c'mon.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2019, 1:39 PM

A lot of the other derived AsmPrinters seem to use lowerCamelCase for private methods not used from the base class, rather than UpperCamelCase. Is that a thing?
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
does not clarify.

ie. should I not be UpperCamelCasing these functions until I use them from the base class?

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

nickdesaulniers edited the summary of this revision. (Show Details)Apr 11 2019, 3:12 PM

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.

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 :)

echristo accepted this revision.Apr 11 2019, 3:27 PM

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

This revision is now accepted and ready to land.Apr 11 2019, 3:27 PM
nickdesaulniers edited the summary of this revision. (Show Details)Apr 11 2019, 3:39 PM
This revision was automatically updated to reflect the committed changes.