[libcxx-commits] [PATCH] D129051: [libc++] Make parameter names consistent and enforce the naming style using readability-identifier-naming

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__format/formatter.h:76
+_LIBCPP_HIDE_FROM_ABI constexpr char __hex_to_upper(char __c) {
+  switch (__c) {
   case 'a':
----------------
philnik wrote:
> Mordante wrote:
> > Nice catch! Can you undo this change? It will be removed in D128846.
> I'd rather keep this change in, since clang-tidy would fail otherwise or we couldn't enforce the proper naming.
I already landed the patch so this code no longer exists.

I really like these cleanup patches, but I really would like to urge you to be careful about creating merge conflicts in actively in flight patches. I don't ask you to actively search for them, but please ping people when the patch touches code in areas other's are working on. We can land this patch without enforcing the clang-tidy test and enable the test at a later time.


================
Comment at: libcxx/include/regex:2971
 
-    bool __test_back_ref(_CharT c);
+    bool __test_back_ref(_CharT);
 
----------------
philnik wrote:
> Mordante wrote:
> > I have a preference to use `__c`, without a named argument it almost sounds like the argument is unused.
> My general approach is that if the name isn't useful to the reader in the declaration just don't add one. Saying "this char is really a character" isn't exactly useful IMO. I've also removed some `path p` in filesystem somewhere. In general I'm assuming that arguments are used, especially in implementation-detail functions. Our code-base isn't so large that it would be impractical to remove an argument everywhere in a single patch.
I disagree. I really dislike the idea just to remove variables names because we can. Names have meaning for the reader even simple names. When there's no name you don't know whether it just was a simple name or a special name just removed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129051