search

cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
17 stars 21 forks source link

Disable Clang-Tidy by default #923

Closed agarny closed 3 years ago

agarny commented 3 years ago

Right now, if you have Clang-Tidy installed on your system then our build system will find it and set LIBCELLML_CLANG_TIDY to ON. However, to build libCellML with Clang-Tidy is very slow. So, it would be good to have it disabled by default.

hsorby commented 3 years ago

We will need to modify the CI as this will have an affect there. This will need to be done before this can be merged in otherwise the clang tidy check will no longer function.

agarny commented 3 years ago

Fair enough. Looking forward to the CI being updated then. :)

nickerso commented 3 years ago

Seems like #347 was fixed but incorrectly set the default to on :)

agarny commented 3 years ago

Oopsie indeed!

hsorby commented 3 years ago

I have updated the CI. This one can be dealt with now. We should check afterwards that the CI is indeed working of course and not just take my word for it.

agarny commented 3 years ago

If I compare an old build with a new build, I can see that you now pass -DCLANG_TIDY=TRUE, so that looks good to me. (Not sure we can further test things without "upsetting" Clang-Tidy. Might give it a try!)

agarny commented 3 years ago

Testing Clang-Tidy in our CI machine through ee94ffb.

agarny commented 3 years ago

Otherwise, I noticed that our Clang-Tidy checks are somewhat weak (nowadays?). For instance:

ModelPtr otherParent = std::dynamic_pointer_cast<Model>(units->parent());

should be replaced with:

auto otherParent = std::dynamic_pointer_cast<Model>(units->parent());

but now, both are allowed.

Same with:

if (x != nullptr)...

and

if (x) ...

Both are allowed...!?

agarny commented 3 years ago

Ok, Clang-Tidy is properly set on our CI machine. 🥳