apache / orc

Apache ORC - the smallest, fastest columnar storage for Hadoop workloads
https://orc.apache.org/
Apache License 2.0
670 stars 477 forks source link

[C++] uniform identifiers naming style. #1845

Closed ffacs closed 3 months ago

ffacs commented 4 months ago
          Unrelated to this PR: we have many weird `_xxx` variable names appearing in the function signature. I'm thinking to do something like apache/arrow, which only use `xxx_` as the name of private class member variables. In this way, we can get rid of the weird case like this.

_Originally posted by @wgtmac in https://github.com/apache/orc/pull/1761#discussion_r1468513025_

ffacs commented 4 months ago

The following .clang-tidy file can help us to check naming styles mismatch. And we can use clang-tidy to fix them.

Checks: "-*,
        readability-identifier-naming,
"

CheckOptions:
  [
      { key: readability-identifier-naming.PrivateMemberSuffix,   value: "_"  },
  ]

WarningsAsErrors: ''
HeaderFilterRegex: ''
FormatStyle:     none

But there is a problem that some protected/public variables need to be initialized on construction, what should the naming of corresponding parameter should be? What do you think? @wgtmac @dongjoon-hyun

wgtmac commented 4 months ago

Could we add NOLINT to those public/protected variables?

ffacs commented 4 months ago

Could we add NOLINT to those public/protected variables?

Sorry for my confusing expression. What I mean just like below https://github.com/apache/orc/blob/e1f185eedd7dc0d9d60339117c9299f5b26ddeaf/c%2B%2B/src/ConvertColumnReader.cc#L26-L33

The readType is a protected member of ConvertColumnReader. if we still use readType as the name, we can`t uniform the naming of constructor parameter.

wgtmac commented 4 months ago

What about this?

dongjoon-hyun commented 4 months ago

Can we borrow some existing styles from other open source projects? IIRC, initially, @wgtmac mentioned Arrow community. Are the above rules came from Arrow community?

dongjoon-hyun commented 4 months ago

For the record, I welcome any linter-based rules. Thank you for making this efforts, @ffacs and @wgtmac .

wgtmac commented 4 months ago

We could leverage https://clang.llvm.org/extra/clang-tidy/checks/readability/identifier-naming.html for variable naming. Apache Arrow used cpplint approach which we cannot borrow: https://github.com/apache/arrow/blob/main/cpp/build-support/cpplint.py