[QST] Discrepancies between the style defs in cpp/.clang-tidy and raft common practice

[achirkin]

Since a few month ago I've been able to use clang-tidy (CT) on most of the raft's files, and I've noticed its suggestions in the naming style differ from what I'm used to see in raft as common practice (CP). I haven't calculated the statistics of CP, so I'm writing down my subjective feeling about the matter:

1. Template parameters: CT: camel case CP: snake case seems to be more common
2. enum type names: CT: camel case CP: both variants observed
3. enum constant names: CT: camel case with k prefix CP: camel or all caps, but never with the k prefix
4. constexpr "variables": CT: camel case with k prefix CP: anything possible

These are the ones disturbing me when I need to make a decision. Please, add more items as needed.

My main question is, (how) shall we modify .clang-tidy rules to better reflect common preferences?

[MatthiasKohl]

You can take a look at the .clang-tidy in cugraph-ops: I've worked for many hours on that file, trying to make it somewhat compatible with my own conventions.

Note that your issue is essentially only talking about the config of the readability-identifier-naming check, which has of course the largest config, but there are many other checks, you can see an explanation of all excluded checks in the linked file as well.

On the points you mentioned specifically, cugraph-ops does the following:

1. For template parameters that are constants (int etc.): UPPER_CASE . For template paramters that are types: CamelCaseT
2. enum type names: CamelCase
3. enum constant names: kCamelCase [I don't have a strong opinion here, we don't use enums that extensively]
4. constexpr variables: UPPER_CASE, as with template parameter constants

Talking about naming conventions specifically, I've only really encapsulated my personal style and possibly @teju85's style to some extent. You can take a look at the code in cugraph-ops, and IMO, it looks quite good, but this is definitely not the only way of formatting things.

[MatthiasKohl]

Concerning your questions:

Please, add more items as needed.

There are also type aliases, which in cugraph-ops are lower_case_t, and RAFT might not follow any convention here.

How should we modify .clang-tidy rules to better reflect common preferences?

I think this is the wrong question to ask. Someone simply needs to take a decision, rather than trying to adapt rules to common practice (which will result in no action at all because no rule will be able to capture all the preferences).

This is why I was very adamant on this point in cugraph-ops, since I had the possibility to enforce it before the API got public, and now it will become harder and harder to change this rule-set because other libraries become dependent on the API. Since we now also have support for clang-tidy in CI, almost no-one will argue against it because we delegate the responsibility of enforcing the rule to "the machine", rather than myself. You might think that this is very dictatorship-like, and it certainly is, but in the end, no-one (including me) cares all that much about any given naming convention, but we want it to be consistent across the library, and maybe even across parts of C++ projects in general.

For RAFT, if you start enforcing any good rule-set now, it will change a significant part of the API of RAFT, it will be a nightmare. The longer we wait, the worse the impact is. Add to this that it's currently impossible to run clang-tidy in CI due to cuco and libcudacxx dependencies. RAFT might already be beyond the point of no return, so I'm not sure whether it's worth spending time on this anymore.

[github-actions[bot]]

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

[github-actions[bot]]

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.