This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Make parameter names consistent and enforce the naming style using readability-identifier-naming
ClosedPublic

Authored by philnik on Jul 3 2022, 3:07 PM.

Details

Summary

Ensure that parameter names have the style __lower_case

Diff Detail

Event Timeline

philnik created this revision.Jul 3 2022, 3:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2022, 3:07 PM
Herald added a subscriber: miyuki. · View Herald Transcript
philnik requested review of this revision.Jul 3 2022, 3:07 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 442146.Jul 4 2022, 2:31 PM
  • Try to fix CI

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 ↗(On Diff #442146)

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
684

I don't feel this aids the readability, so I would prefer to keep it as-is.
Instead rename _Last to __l.
In general it's good to use longer names for the larger scope.

719

Likewise please keep this __base.

libcxx/include/regex
2966

I have a preference to use __c, without a named argument it almost sounds like the argument is unused.

philnik retitled this revision from [libc++] Make parameter names consistent and enforce the naming style to [libc++] Make parameter names consistent and enforce the naming style using readability-identifier-naming.Jul 6 2022, 6:07 AM
philnik updated this revision to Diff 442544.Jul 6 2022, 6:09 AM
philnik marked 2 inline comments as done.
  • Address comments
libcxx/include/__format/formatter.h
76 ↗(On Diff #442146)

I'd rather keep this change in, since clang-tidy would fail otherwise or we couldn't enforce the proper naming.

libcxx/include/regex
2966

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.

Mordante added inline comments.Jul 7 2022, 9:58 AM
libcxx/include/__format/formatter.h
76 ↗(On Diff #442146)

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
2966

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.

philnik updated this revision to Diff 442990.Jul 7 2022, 10:50 AM
philnik marked 3 inline comments as done.
  • Rebased
  • Try to fix CI
libcxx/include/__format/formatter.h
76 ↗(On Diff #442146)

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.

libcxx/include/regex
2966

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?

ldionne accepted this revision.Jul 8 2022, 7:12 AM
ldionne added inline comments.
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
727

This shuffling around of variables names is tricky, but I checked and I think it's correct.

libcxx/include/regex
2966

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.

This revision is now accepted and ready to land.Jul 8 2022, 7:12 AM
This revision was automatically updated to reflect the committed changes.
philnik marked 4 inline comments as done.