Change coding style for argparse

[p-ranav]

• Change private member variable names to be (1) underscore separated, (2) all lower case, (3) with a trailing underscore, e.g., my_private_variable_ instead of mMyPrivateVariable.
• Change member function names to be (1) underscore separated, (2) all lower case, (3) with a trailing underscore if the member function is private, e.g., my_public_member_function(), and my_private_member_function_()
• Change function parameter names to be (1) underscore separated, (2) all lower case, e.g., my_function(int foo, float bar)
• Class names will be PascalCase, e.g., ArgumentParser
• Add clang-format configuration and use the standard LLVM coding style

From:

  std::vector<std::string> mNames;
  std::string_view mUsedName;

To:

  std::vector<std::string> names_;
  std::string_view used_name_;

[skrobinson]

I'm not a fan of external underscores for two reasons: (1) more easily visually missed; (2) I've seen an editor that chopped off the bottoms of rows of text with tall characters (e.g. _('我們') would not show the underscore). A common alternative for member variables is a leading m_. My preference is m_.

I tend to look for the preceding private/public/protected keyword for functions rather than relying on naming. So I'm less concerned with this change hiding details about the function and I wonder if there's much value in a different convention for private member functions. My preference is for no special naming of private member funtions.

We have a mix of class and struct. Should these be consolidated? If not, should their respective styles help differentiate them?

Add clang-format configuration and use the standard LLVM coding style

It looks like you have a .clang-format file in the project root.

[p-ranav]

I'm okay with m_ prefix for member variables. I'm also okay with no special naming of private member functions since I agree with your points. I'm not particularly bothered by the mixed use of class and struct - the naming style was meant for both classes and structs. In your experience, is there a reason to treat structs and classes (w.r.t naming style) differently?

Additionally, I've been wanting to break argparse.hpp into a handful of smaller header files - can generate the single header version (like here). That would seriously improve readability and maintainability of the source.

[skrobinson]

I'm not particularly bothered by the mixed use of class and struct - the naming style was meant for both classes and structs. In your experience, is there a reason to treat structs and classes (w.r.t naming style) differently?

When first reading argparse.hpp, I noticed how the structs seemed to be "active", while the two classes model objects. The style conventions (snake_case vs PascalCase) raised my awareness of this difference. I just found it an interesting split, but I do not have a preference either way.

Additionally, I've been wanting to break argparse.hpp into a handful of smaller header files

As long as a single-file version is available without user processing, I don't see a problem. I've never worked with a split header project so I don't see the benefit. Could you explain how it helps?

[p-ranav]

Could you explain how it helps?

It helps with a maintainability (see nlohmann/json example below) and flexibility w.r.t how the source is consumed (see libfmt example below).

The change would enable us to maintain a few small header files, each solving a one or more problems. argparse.hpp is over 1100 lines long right now (not large but still). We could break that up into a few smaller files, e.g., argument.hpp, argument_parser.hpp, parse_number.hpp, stream_helpers.hpp, etc. It would also help with reviewing isolated parts of the code for improvements.

Reading code is hard when everything is in one file.

A good example on this would be nlohmann/json: The single include version is 26,647 lines long. The authors have broken the project down into multiple headers (here), each header provides solutions to a small subset of the problems, e.g., converters, parsers, iterators, serializers etc.

Another example would be libfmt where the authors provide a macro FMT_HEADER_ONLY if users want a header-only version. Otherwise, the code is built as a shared library. Breaking the code apart allows for such flexibility.

[skrobinson]

First, would this discussion be better on a Wiki page so that there is one copy of the most current style guide?

Proposed final checks:

Checks:
  -*,
  clang-analyser-*,
  cppcoreguidelines-avoid-c-arrays,
  cppcoreguidelines-special-member-functions,
  readability-*,

CheckOptions:
  - { key: readability-identifier-naming.ClassCase, value: CamelCase }
  - { key: readability-identifier-naming.FunctionCase, value: lower_case }
  - { key: readability-identifier-naming.NamespaceCase, value: lower_case }
  - { key: readability-identifier-naming.PrivateMemberPrefix, value: m_ }
  - { key: readability-identifier-naming.StructCase, value: CamelCase }
  - { key: readability-identifier-naming.VariableCase, value: lower_case }

HeaderFilterRegex: '.*'

The following checks match the current argparse style. In summary, turn off all checks, turn on all readability checks, disable some readability checks, and set options for some identifier naming.

Checks:
  -*,
  readability-*,
  -readability-braces-around-statements,
  -readability-container-size-empty,
  -readability-else-after-return,
  -readability-function-cognitive-complexity,
  -readability-magic-numbers,
  -readability-make-member-function-const,
  -readability-named-parameter,
  -readability-qualified-auto,
  -readability-static-accessed-through-instance,

CheckOptions:
  - { key: readability-identifier-naming.ClassCase, value: CamelCase }
  - { key: readability-identifier-naming.ConstexprVariableCase, value: lower_case }
  - { key: readability-identifier-naming.FunctionCase, value: lower_case }
  - { key: readability-identifier-naming.LocalVariableIgnoredRegexp, value: "^[a-z][a-z_]+" }
  - { key: readability-identifier-naming.NamespaceCase, value: lower_case }
  - { key: readability-identifier-naming.PrivateMemberPrefix, value: m }
  - { key: readability-identifier-naming.StructCase, value: lower_case }
  - { key: readability-identifier-naming.VariableCase, value: camelBack }

HeaderFilterRegex: '.*'

[p-ranav]

Sounds good to me. And yeah, I can add it to the Wiki once merged.

[skrobinson]

The conversions are going well enough that I'm not sure the Wiki will be needed. I think I'll have less than three PR to finish up this issue and the transition time will be relatively short.

I have hit a few identifier naming edge cases that will need decisions.

Should function arguments still start with a? Should function-scope identifiers still start with t? See ArgumentParser::parse_args_internal for a function with both. As I see it, the reason for m_ is to flag a name with larger scope than just inside member functions. I would choose no special prefixes for local names, but I don't have a strong opinion either way.

When renaming is_container to IsContainer, what to do with is_container_v?

parse_number is a struct, but is only used as a callable (i.e. no member functions). Shoule this become ParseNumber?

[p-ranav]

I agree with you on naming temporary variables/function args.

• We can remove the a prefix on function arguments and t prefix on function-scope identifiers.

  • This was the naming scheme used by the platoengine - one that followed me for a few years when I first started working on argparse. I do not like the naming scheme anymore and we can move away from it.

• Maybe we can change is_container to HasContainerTraits and is_container_v to IsContainer. Then, the code would read like this:

template <typename T>
static constexpr bool IsContainer = HasContainerTraits<T>::value;

• We can keep parse_number as is since we're using it as a callable - We could just define it as a function template with necessary specializations.

[skrobinson]

change is_container to HasContainerTraits and is_container_v to IsContainer

This seems to work.

We can keep parse_number as is since we're using it as a callable - We could just define it as a function template with necessary specializations.

I suspect parse_number is implemented as it is to get around function template partial specialization, which is not supported in the language. I recommend we don't touch parse_number.