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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/__filesystem/operations.h:42
 _LIBCPP_FUNC_VIS void __copy(const path& __from, const path& __to, copy_options __opt, error_code* __ec = nullptr);
-_LIBCPP_FUNC_VIS bool __create_directories(const path& p, error_code* ec = nullptr);
+_LIBCPP_FUNC_VIS bool __create_directories(const path&, error_code* = nullptr);
 _LIBCPP_FUNC_VIS void __create_directory_symlink(const path& __to, const path& __new_symlink, error_code* __ec = nullptr);
----------------
In the other functions we use `__ec = nullptr`, let's keep that for consistency. This applies to several functions you modified here.


================
Comment at: libcxx/include/charconv:727
         [](const char* __p, const char* __lastp, _Tp& __val,
-           int _Base) -> from_chars_result {
+           int __b) -> from_chars_result {
             using __tl = numeric_limits<_Tp>;
----------------
This shuffling around of variables names is tricky, but I checked and I think it's correct.


================
Comment at: libcxx/include/regex:2971
 
-    bool __test_back_ref(_CharT c);
+    bool __test_back_ref(_CharT);
 
----------------
philnik wrote:
> 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?
I'd tend to agree with @philnik on this one. We shouldn't go around eagerly removing parameter names just because we can, however I see little benefit to keeping names that really don't bring any additional semantical information, like it's the case for `__c` here. We already know what the parameter is, it's a char because the type is `CharType`.

All in all, this is a small issue and I'm OK with either approach, but if this were my patch, I would probably have removed the parameter name because it's what makes most sense to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129051