Ensure that parameter names have the style __lower_case
Details
- Reviewers
ldionne Mordante var-const jdoerfert - Group Reviewers
Restricted Project - Commits
- rGb48c5010a462: [libc++] Make parameter names consistent and enforce the naming style using…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Thanks for the cleanup! Some small comments.
Did you use clang-tidy to find these? If so maybe add an attribution in the commit message.
Since the CI fails I would like to have a look at the next version.
libcxx/include/__format/formatter.h | ||
---|---|---|
76 | Nice catch! Can you undo this change? It will be removed in D128846. | |
libcxx/include/__format/formatter_output.h | ||
36–37 | This is the duplicate and this version will be the only one after D128846. | |
libcxx/include/charconv | ||
619 | I don't feel this aids the readability, so I would prefer to keep it as-is. | |
654 | Likewise please keep this __base. | |
libcxx/include/regex | ||
2971 | I have a preference to use __c, without a named argument it almost sounds like the argument is unused. |
- Address comments
libcxx/include/__format/formatter.h | ||
---|---|---|
76 | I'd rather keep this change in, since clang-tidy would fail otherwise or we couldn't enforce the proper naming. | |
libcxx/include/regex | ||
2971 | 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. |
libcxx/include/__format/formatter.h | ||
---|---|---|
76 | 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. | |
libcxx/include/regex | ||
2971 | 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. |
- Rebased
- Try to fix CI
libcxx/include/__format/formatter.h | ||
---|---|---|
76 | 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. | |
libcxx/include/regex | ||
2971 | 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. |
libcxx/include/__filesystem/operations.h | ||
---|---|---|
42 | In the other functions we use __ec = nullptr, let's keep that for consistency. This applies to several functions you modified here. | |
libcxx/include/charconv | ||
662 | This shuffling around of variables names is tricky, but I checked and I think it's correct. | |
libcxx/include/regex | ||
2971 | 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. |
In the other functions we use __ec = nullptr, let's keep that for consistency. This applies to several functions you modified here.