Skip to content

Add linting with clang tidy #39

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Add linting with clang tidy #39

wants to merge 6 commits into from

Conversation

alexander-novo
Copy link
Collaborator

@alexander-novo alexander-novo commented Jan 6, 2025

Was unfortunately unable to find a lint that enforced the MicroGrid namespace, but I'll keep looking.

There are a lot of things that need to be fixed. I'll fix them as a separate pull request.

I turned off the "magic numbers" lint just because I think the mathematical nature of the code will require some magic numbers. I also turned off the "easily swappable parameters" lint because it seemed too burdensome to fix properly, although please check the documentation for the lint and let me know if it's something we should have on.

I don't currently have any "modernize" lints on (lints which encourage using more modern features of C++ over older methods), but I have considered enabling the following:

Closes #34

@alexander-novo alexander-novo requested a review from pelesh January 6, 2025 22:31
@pelesh
Copy link
Collaborator

pelesh commented Jan 9, 2025

@alexander-novo, could you explain what this PR accomplishes?

@pelesh
Copy link
Collaborator

pelesh commented Jan 17, 2025

I ran lint and got ~6,000 lines of suggestions, most of them actually very helpful. Thanks, @alexander-novo!

I can see one possible showstopper: Enforcing variable names to abide by the style guidelines works strict to the letter, but in some cases it would be good to have exceptions. For example, variables Eq and Ed are readily recognized as quadrature and direct potentials, $E_q$ and $E_d$, and make equation code easier to read. If we are to adapt these names to the current rules, we would have to write them as e_q and e_d. Is it possible to (at least) ignore first capital letter in variable names? You would want your variable names in your code to be similar to math symbols in the equations.

@abirchfield, please chime in.

@pelesh pelesh requested a review from abirchfield January 17, 2025 18:48
@pelesh
Copy link
Collaborator

pelesh commented Jan 17, 2025

I turned off the "magic numbers" lint just because I think the mathematical nature of the code will require some magic numbers.

I would say that mathematical nature of the code would be the reason to eliminate all magic numbers. We want all numbers to be defined before we use them. But, I can also see how it can backfire. Lint could catch on ,e.g., $\frac{1}{2}$ in the expression for the kinetic energy $\frac{mv^2}{2}$, so we would need to code it something like

constexpr real_type ONE_HALF = static_cast<real_type>(0.5);

(...)

e_k = ONE_HALF * m * v * v

Perhaps we can live with this?

Copy link
Collaborator

@abirchfield abirchfield left a comment

Choose a reason for hiding this comment

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

I am OK with this. Most of this is a matter of preference. My preference would be to write 0.5 rather than ONE_HALF, and to align variable capitalization with modeling conventions. But it looks like we can add exceptions as needed.

@alexander-novo
Copy link
Collaborator Author

alexander-novo commented Feb 9, 2025

I ran lint and got ~6,000 lines of suggestions, most of them actually very helpful. Thanks, @alexander-novo!

I can see one possible showstopper: Enforcing variable names to abide by the style guidelines works strict to the letter, but in some cases it would be good to have exceptions. For example, variables Eq and Ed are readily recognized as quadrature and direct potentials, E q and E d , and make equation code easier to read. If we are to adapt these names to the current rules, we would have to write them as e_q and e_d. Is it possible to (at least) ignore first capital letter in variable names? You would want your variable names in your code to be similar to math symbols in the equations.

@abirchfield, please chime in.

It is possible to disable checking of identifier names which match a certain regular expression, so we could set this regular expression which matches any identifier which begins with a capital letter. But I really think variables really should start with a lowercase letter - when I see an identifier that begins with a capital letter, I think that it refers to a class or struct. Also, ignoring a variable name will also disable other checks, such as ensuring that member variables have an underscore suffix.

We could also introduce a prefix such as, for instance, m_ which signifies that the variable itself has some significance in a math formula, and then use that prefix as the regular expression. In this system, the given variables would be called m_Ed and m_Eq.

I also worry that there are plenty of math notations that we wouldn't be able to accurately represent in a variable name, so it may be better to go with the prefix m_ (or no prefix) to denote a variable which corresponds to a specific variable in a formula and then require a documentation comment showing how to format that variable in LaTeX? We have other LaTeX doc-comments in the code already.

Constants 2.0 and 0.5 are ignored, since these are common and self-explanatory constants in many formulae.
@pelesh
Copy link
Collaborator

pelesh commented Feb 10, 2025

We could also introduce a prefix such as, for instance, m_ which signifies that the variable itself has some significance in a math formula, and then use that prefix as the regular expression. In this system, the given variables would be called m_Ed and m_Eq.

Using m_ might be confusing because some codes use m_ to denote class member variables. In mathematical expressions we could perhaps use prefix v_ to denote variables in the equation (e.g. v_Eq), and prefix p_ to denote parameters, e.g. p_Xqq. That way, prefixes would add some information that could help coder parse equations more easily.

@alexander-novo
Copy link
Collaborator Author

I added an exceptional local variable name _ for variables which are never used (and therefore don't really need a good name), but for grammatical reasons must exist (e.g. in range-based for loops).

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.

Add linting
3 participants