[libcxx-commits] [PATCH] D129386: [libc++] Make the naming of private member variables consistent and enforce it through readability-identifier-naming

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/.clang-tidy:46
+  - key:   readability-identifier-naming.PrivateMemberSuffix
+    value: _
 
----------------
Mordante wrote:
> Mordante wrote:
> > Not your change but please verify whether https://reviews.llvm.org/D129503#inline-1244548 is addressed otherwise please rebase and fix that before landing this patch.
> Maybe not this patch but can we add a regex that when a member starts with `__` it should end with `_`.
I'll try, but that might require a custom clang-tidy check. I think we should first work through the checks already available before adding custom ones for us. It might work with `PublicMemberCase` combined with `PublicMemberIgnoredRegexp` though. I'll try that after the easier ones.


================
Comment at: libcxx/include/__chrono/year.h:105
 bool year::ok() const noexcept
-{ return static_cast<int>(min()) <= __y && __y <= static_cast<int>(max()); }
+{ return static_cast<int>(min()) <= __y_ && __y_ <= static_cast<int>(max()); }
 } // namespace chrono
----------------
Mordante wrote:
> This will conflict with D129442, sorry I wasn't aware of this patch.
I discovered the tautological compare while working on this patch, so I already expected a merge-conflict. It's small enough that I thought it wouldn't matter that much.


================
Comment at: libcxx/include/any:315
         void *  __ptr;
         __any_imp::_Buffer __buf;
     };
----------------
Mordante wrote:
> This prompted my regex question, since they weren't updated.
With this patch I'm only checking private members, since protected or public members could be required by the standard. I'm working on checking more, but it'll take some time to get most things checked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129386