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

Make Clang-Tidy happy #596

Merged
merged 1 commit into from
May 13, 2020
Merged

Make Clang-Tidy happy #596

merged 1 commit into from
May 13, 2020

Conversation

agarny
Copy link
Contributor

@agarny agarny commented May 12, 2020

Addresses issue #595.

@agarny agarny requested review from hsorby, kerimoyle and nickerso and removed request for hsorby and kerimoyle May 12, 2020 04:33
@agarny
Copy link
Contributor Author

agarny commented May 12, 2020

@hsorby, @nickerso and @kerimoyle : it should literally take less than 2 minutes to review this.

@nickerso
Copy link
Contributor

are any of our CI systems using this new version of Clang tidy?

@agarny
Copy link
Contributor Author

agarny commented May 12, 2020

I would think not unless @hsorby updated our CI Mac?

@agarny
Copy link
Contributor Author

agarny commented May 12, 2020

Actually, trying to connect to the Mac, I am getting told that libCellML is using the display. Why?! (This is not the first time that I see that.)

Anyway, I have logged in, and Homebrew was still locked since the end of January (!?). I removed the locks and tried to upgrade Homebrew, but now I am being told that I don't have write access to some files... because things were previously updated and file access is set to 755...!?

Anyway nb 2, Screen Sharing has gone completely black on me. Giving up. (I tried to reconnect and I am once again being told that libCellML is using the display...!?)

@agarny
Copy link
Contributor Author

agarny commented May 12, 2020

FTR, our CI Mac is currently doing a brew upgrade. So, just a matter of time before Clang-Tidy gets updated to 10.0.0.

@agarny
Copy link
Contributor Author

agarny commented May 12, 2020

Ok, FWIW, the CI Mac now uses Clang-Tidy 10.0.0 and I have just retriggered the tests. So, it's all good for you, @nickerso, @hsorby and @kerimoyle, to review and merge in (so that we can then onto PR #599 and then PR #598 (in that order).

@kerimoyle
Copy link
Contributor

kerimoyle commented May 12, 2020

Does this mean that our own machines also need to be upgraded? And what do the new flags surrounding auto mean we need to do differently?

@agarny
Copy link
Contributor Author

agarny commented May 12, 2020

No, if you have an older version of Clang-Tidy (as was the case for our CI Mac) then it will still be fine. If you don't have Clang-Tidy installed on your system, our build system will skip some checks, so fine again.

It's just that my copy of Clang-Tidy is up to date, so I can't build the develop version of libCellML unless I disable Clang-Tidy. So, this PR fixes our build system so that it not only works with "old" versions of Clang-Tidy, but also with the latest one.

Copy link
Contributor

@kerimoyle kerimoyle left a comment

Choose a reason for hiding this comment

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

Works ok for me.

@nickerso
Copy link
Contributor

Do we need to document why we choose a given configuration option for this? or is it sufficient to say we configure Clang Tidy to match our style guide and assume that people know what they are doing? (Just asking as these options being added mean nothing to me so don't feel I can review it if by doing so would imply that I agree to these options.)

@agarny
Copy link
Contributor Author

agarny commented May 12, 2020

I don't know if it is worth documenting this somewhere. Our code mentions that the full list of Clang-Tidy checks can be found at https://clang.llvm.org/extra/clang-tidy/checks/list.html. When it comes to the ones that I have added, you can see their description at https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-init-variables.html and at https://clang.llvm.org/extra/clang-tidy/checks/readability-qualified-auto.html.

The reason I have added those "two" checks is that for the initial variables check, it would require changing a hell of a lot of the code (something that we might want to do at some point though?) while for the second one I thought it made the use of auto somewhat cumbersome. I mean, that check wants you to use auto * for pointers while the whole point of auto for me is that I don't have to second-guess the type of the variable I am dealing with.

Open to suggestions though...

Copy link
Contributor

@nickerso nickerso left a comment

Choose a reason for hiding this comment

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

These changes seem to be working on the one CI machine that has an updated clang tidy, so happy to approve them. We may want to review our Clang Tidy config at some point in the future...

@nickerso nickerso merged commit 2e34152 into cellml:develop May 13, 2020
@agarny agarny deleted the issue595 branch May 13, 2020 01:39
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.

None yet

3 participants