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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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':
----------------
Mordante wrote:
> 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.
I don't want to create more work for people, but I think there is a balance here. If you would have said that this causes problems in multiple places I would have been happy to wait for the other patches to land first. OTOH enforcing proper naming in the CI outweighs having to resolve a single merge conflict IMO.
I can try to ping people in future patches, but there would probably be a high false-positive rate. Most of the clang-tidy cleanups touch a lot of files, but only a few lines per file (other than really bad offenders like `locale_win32.h`). This makes it very hard to determine if anybody is actually touching that code and I would probably have to ping everyone who is working on libc++ currently in this patch.
Hui and Konstantin because of the changes in the algorithms; you because of the changes in format and I don't really know what Louis is working on, but there is probably something in here.


================
Comment at: libcxx/include/regex:2971
 
-    bool __test_back_ref(_CharT c);
+    bool __test_back_ref(_CharT);
 
----------------
Mordante wrote:
> 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. 
Having a simple name could just as well mean that someone didn't think of a good name or didn't update it in the declaration. I don't think having `__c` there helps me in any way to know that. I'm not saying that we should just remove all argument names. I'm saying that single-letter names or names that are essentially just the type (like `basic_string __str`) are useless to the reader and should be omitted.

As an example, this is from `<string>`:
```
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(const basic_string&, const basic_string&);
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(const value_type*, const basic_string&);
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(value_type, const basic_string&);
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(const basic_string&, const value_type*);
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(const basic_string&, value_type);

vs.

friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(const basic_string& __x, const basic_string& __y);
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(const value_type* __x, const basic_string& __y);
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(value_type __x, const basic_string& __y);
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(const basic_string& __x, const value_type* __y);
friend _LIBCPP_CONSTEXPR_AFTER_CXX17 basic_string operator+<>(const basic_string& __x, value_type __y);

```
IMO the extra names are just noise which make the declarations harder to read.
@ldionne What's your take on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129051