Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a clang-tidy check in the regular CI run #3620

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

randombit
Copy link
Owner

No description provided.

@randombit randombit marked this pull request as draft July 10, 2023 11:06
@@ -21,6 +21,7 @@
'modernize-use-nullptr',
'readability-braces-around-statements',
'performance-unnecessary-value-param',
'*-non-private-member-variables-in-classes',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #3619 you converted some struct to class. Was that also due to a clang-tidy rule by any chance? If yes: maybe it's worth adding that here as well. Mea culpa. 👼

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a clang-tidy rule about this (though if there is I'd like to enable it).

The logic here is: plain struct, if ever used, should just be a C-style struct. If it has a constructor or associated logical functions, it should be a class. Also, unless there truly is no logical consistency requirements (which is rare!), it should be a class, and any mutations performed through setters. This allows easier understanding of how/when something is modified. Eg if the member variable is just public, then it's very easy to capture it by mutable reference in a way that is not so obvious:

struct S { int foo; int something; }

int bar(S& s) { 
  s.something = 9;
  return baz(s.foo);
}

/// in some other file far away ...
int baz(int& x) { x = 5; return 9; }

whereas if there are getters and setters than mutations are highly visible/grepable. Also in many cases no setter is required (for example the identity field in Client_PSK) in which case the absence of a setter at all makes it clear this field is never modified.

@randombit randombit marked this pull request as ready for review July 11, 2023 09:51
@randombit randombit changed the title Draft: clang-tidy in standard CI Add a clang-tidy check in the regularly CI run Jul 11, 2023
@randombit randombit changed the title Add a clang-tidy check in the regularly CI run Add a clang-tidy check in the regularl CI run Jul 11, 2023
@randombit randombit changed the title Add a clang-tidy check in the regularl CI run Add a clang-tidy check in the regular CI run Jul 11, 2023
@randombit randombit merged commit 601a5f0 into master Jul 11, 2023
32 of 33 checks passed
@coveralls
Copy link

Coverage Status

coverage: 91.74% (+0.02%) from 91.725% when pulling 65d3589 on jack/fast-clang-tidy-in-ci into c6a09af on master.

@randombit randombit deleted the jack/fast-clang-tidy-in-ci branch July 13, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants