You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/11/04 22:18:09 UTC
ClangTidy for Mesos is here!
We've had ClangTidy for Mesos, called MesosTidy for a while checked
into the codebase, but we hadn't actually enabled it on the CI.
I've created a Mesos-Tidybot job (to accompany Mesos-Buildbot) on
our Apache CI, which will be reporting its status to the builds mailing
list.
The following is an example output from MesosTidy in its last run:
/tmp/SRC/3rdparty/libprocess/src/http.cpp:675:10: warning: redundant call
to 'data' [readability-redundant-string-cstr]
body = out.str().data();
^~~~~~~~~~~~~~~~~
out.str()
This warning is generated from a built-in ClangTidy check called
readability-redundant-string-cstr
<http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-cstr.html>,
which checks for unnecessary
calls to `std::string::c_str` and `std::string::data`.
Not a critical issue for us by any measure, but it's very important to
note that this is a __semantic__ check and I hope it gives you
a sense of the type of checks that ClangTidy is capable of.
Another thing to note is that it even suggests a fix!
Namely, to change `out.str().data()` to `out.str()`.
I've put up a review to make this change here: r63560
<https://reviews.apache.org/r/63560/>
Today, we start with a very small number of checks:
- `mesos-flags-inheritance`
- Ensures that Flags always inherits virtually for composability.
- `mesos-namespace-comments`
- Ensures that namespaces end with `// namespace foo {`
- `readability-redundant-string-cstr`
- As we saw above.
We also have a `mesos-this-capture` check which will tell you when
you've captured the `this` pointer of a class in a libprocess continuation
but without a `defer` to `self`. This is disabled today however, since there
are few false positives related to the uses of `process::loop`. I really
hope
to get around to updating the check and enable it soon!
There are also a slew of checks that ship with ClangTidy that we'll be
enabling as well. http://clang.llvm.org/extra/clang-tidy/checks/list.html
Notably a lot of the `google-*` checks are relevant for us.
You can also run `mesos-tidy` locally on your machine by running
`./support/mesos-tidy.sh`.
Let's say this is an alpha launch. There are lots of things missing,
and things aren't going to be perfect, but I think that the framework
that is there today is a good start for us. We'll certainly be changing
things and improving as we go.
Thank you to Benjamin Bannier for helping out with so much of this work.
He's been a really big help and an advocate for additional tools.
Thanks,
MPark
Re: ClangTidy for Mesos is here!
Posted by Benjamin Hindman <be...@eecs.berkeley.edu>.
MPark, this is so awesome to see; everyone in the community is indebted to
you for continuing to push on this! Thank you!!!
On Sat, Nov 4, 2017 at 3:18 PM Michael Park <mp...@apache.org> wrote:
> We've had ClangTidy for Mesos, called MesosTidy for a while checked
> into the codebase, but we hadn't actually enabled it on the CI.
>
> I've created a Mesos-Tidybot job (to accompany Mesos-Buildbot) on
> our Apache CI, which will be reporting its status to the builds mailing
> list.
>
> The following is an example output from MesosTidy in its last run:
>
> /tmp/SRC/3rdparty/libprocess/src/http.cpp:675:10: warning: redundant call
> to 'data' [readability-redundant-string-cstr]
> body = out.str().data();
> ^~~~~~~~~~~~~~~~~
> out.str()
>
> This warning is generated from a built-in ClangTidy check called
> readability-redundant-string-cstr
> <
> http://clang.llvm.org/extra/clang-tidy/checks/readability-redundant-string-cstr.html
> >,
> which checks for unnecessary
> calls to `std::string::c_str` and `std::string::data`.
>
> Not a critical issue for us by any measure, but it's very important to
> note that this is a __semantic__ check and I hope it gives you
> a sense of the type of checks that ClangTidy is capable of.
>
> Another thing to note is that it even suggests a fix!
> Namely, to change `out.str().data()` to `out.str()`.
> I've put up a review to make this change here: r63560
> <https://reviews.apache.org/r/63560/>
>
> Today, we start with a very small number of checks:
>
> - `mesos-flags-inheritance`
> - Ensures that Flags always inherits virtually for composability.
> - `mesos-namespace-comments`
> - Ensures that namespaces end with `// namespace foo {`
> - `readability-redundant-string-cstr`
> - As we saw above.
>
> We also have a `mesos-this-capture` check which will tell you when
> you've captured the `this` pointer of a class in a libprocess continuation
> but without a `defer` to `self`. This is disabled today however, since
> there
> are few false positives related to the uses of `process::loop`. I really
> hope
> to get around to updating the check and enable it soon!
>
> There are also a slew of checks that ship with ClangTidy that we'll be
> enabling as well. http://clang.llvm.org/extra/clang-tidy/checks/list.html
> Notably a lot of the `google-*` checks are relevant for us.
>
> You can also run `mesos-tidy` locally on your machine by running
> `./support/mesos-tidy.sh`.
>
> Let's say this is an alpha launch. There are lots of things missing,
> and things aren't going to be perfect, but I think that the framework
> that is there today is a good start for us. We'll certainly be changing
> things and improving as we go.
>
> Thank you to Benjamin Bannier for helping out with so much of this work.
> He's been a really big help and an advocate for additional tools.
>
> Thanks,
>
> MPark
>
Re: ClangTidy for Mesos is here!
Posted by Benjamin Mahler <bm...@apache.org>.
Thanks Michael and Benjamin!
On Sat, Nov 4, 2017 at 3:18 PM, Michael Park <mp...@apache.org> wrote:
> We've had ClangTidy for Mesos, called MesosTidy for a while checked
> into the codebase, but we hadn't actually enabled it on the CI.
>
> I've created a Mesos-Tidybot job (to accompany Mesos-Buildbot) on
> our Apache CI, which will be reporting its status to the builds mailing
> list.
>
> The following is an example output from MesosTidy in its last run:
>
> /tmp/SRC/3rdparty/libprocess/src/http.cpp:675:10: warning: redundant call
> to 'data' [readability-redundant-string-cstr]
> body = out.str().data();
> ^~~~~~~~~~~~~~~~~
> out.str()
>
> This warning is generated from a built-in ClangTidy check called
> readability-redundant-string-cstr
> <http://clang.llvm.org/extra/clang-tidy/checks/readability-
> redundant-string-cstr.html>,
> which checks for unnecessary
> calls to `std::string::c_str` and `std::string::data`.
>
> Not a critical issue for us by any measure, but it's very important to
> note that this is a __semantic__ check and I hope it gives you
> a sense of the type of checks that ClangTidy is capable of.
>
> Another thing to note is that it even suggests a fix!
> Namely, to change `out.str().data()` to `out.str()`.
> I've put up a review to make this change here: r63560
> <https://reviews.apache.org/r/63560/>
>
> Today, we start with a very small number of checks:
>
> - `mesos-flags-inheritance`
> - Ensures that Flags always inherits virtually for composability.
> - `mesos-namespace-comments`
> - Ensures that namespaces end with `// namespace foo {`
> - `readability-redundant-string-cstr`
> - As we saw above.
>
> We also have a `mesos-this-capture` check which will tell you when
> you've captured the `this` pointer of a class in a libprocess continuation
> but without a `defer` to `self`. This is disabled today however, since
> there
> are few false positives related to the uses of `process::loop`. I really
> hope
> to get around to updating the check and enable it soon!
>
> There are also a slew of checks that ship with ClangTidy that we'll be
> enabling as well. http://clang.llvm.org/extra/clang-tidy/checks/list.html
> Notably a lot of the `google-*` checks are relevant for us.
>
> You can also run `mesos-tidy` locally on your machine by running
> `./support/mesos-tidy.sh`.
>
> Let's say this is an alpha launch. There are lots of things missing,
> and things aren't going to be perfect, but I think that the framework
> that is there today is a good start for us. We'll certainly be changing
> things and improving as we go.
>
> Thank you to Benjamin Bannier for helping out with so much of this work.
> He's been a really big help and an advocate for additional tools.
>
> Thanks,
>
> MPark
>