Skip to content

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

Merged
randombit merged 1 commit intomasterfrom
jack/fast-clang-tidy-in-ci
Jul 11, 2023
Merged

Add a clang-tidy check in the regular CI run#3620
randombit merged 1 commit intomasterfrom
jack/fast-clang-tidy-in-ci

Conversation

@randombit
Copy link
Copy Markdown
Owner

No description provided.

@randombit randombit marked this pull request as draft July 10, 2023 11:06
'modernize-use-nullptr',
'readability-braces-around-statements',
'performance-unnecessary-value-param',
'*-non-private-member-variables-in-classes',
Copy link
Copy Markdown
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
Copy Markdown
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 force-pushed the jack/fast-clang-tidy-in-ci branch from 95e1a5f to 65d3589 Compare July 11, 2023 09:49
@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
@coveralls
Copy link
Copy Markdown

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