From: Lars Schneider <larsxschneider@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Samuel Lijin <sxlijin@gmail.com>,
"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH] travis-ci: run scan-build every time
Date: Sun, 26 Feb 2017 00:02:33 +0100 [thread overview]
Message-ID: <70DA368F-97FB-4492-811D-CCDF4F237939@gmail.com> (raw)
In-Reply-To: <20170225223146.ixubnwqkfol5q2gn@sigill.intra.peff.net>
> On 25 Feb 2017, at 23:31, Jeff King <peff@peff.net> wrote:
>
> On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote:
>
>>
>>> On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@gmail.com> wrote:
>>>
>>> Introduces the scan-build static code analysis tool from the Clang
>>> project to all Travis CI builds. Installs clang (since scan-build
>>> needs clang as a dependency) to make this possible (on macOS, also
>>> updates PATH to allow scan-build to be invoked without referencing the
>>> full path).
>>
>> This is a pretty neat idea. However, I think this should become a
>> dedicated job in a TravisCI build (similar to the Documentation job [1])
>> because:
>> a) We don't want to build and test a scan-build version of Git (AFAIK
>> scan-build kind of proxies the compiler to do its job - I don't if
>> this has any side effects)
>> b) We don't want to slow down the other builds
>> c) It should be enough to run scan-build once on Linux per build
>
> Yeah. I am all for static analysis, but I agree it should be its own
> job. Especially as it can be quite noisy with false positives (and I
> really think before any static analysis is useful we need to figure out
> a way to suppress the false positives, so that we can see the signal in
> the noise).
>
> Fully a third of the problem cases found are dead assignments or
> increments. I looked at a few, and I think the right strategy is to tell
> the tool "no really, our code is fine". For instance, it complains
> about:
>
> argc = parse_options(argc, argv, ...);
>
> when argc is not used again later. Sure, that assignment is doing
> nothing. But from a maintainability perspective, I'd much rather have a
> dead assignment (that the compiler is free to remove) then for somebody
> to later add a loop like:
>
> for (i = 0; i < argc; i++)
> something(argv[i]);
>
> which will read past the end of the rearranged argv (and probably
> _wouldn't_ be caught by static analysis, because the hidden dependency
> between argc and argv is buried inside the parse_options() call).
>
> So there is definitely some bug-fixing to be done, but I think there is
> also some work in figuring out how to suppress these useless reports.
That makes sense. I suspected that this assignment was intentional
but I wasn't sure why. I didn't know about the rearrangement of argv.
Apparently an "(void)argc;" silences this warning. Would that be too
ugly to bear? :-)
> Turning off the dead-assignment checker is one option, but I actually
> think it _could_ produce useful results. It just isn't in these cases.
> So I'd much rather if we can somehow suppress the specific callsites.
>
>> I ran scan-build on the current master and it detected 72 potential bugs [2].
>> I looked through a few of them and they seem to be legitimate. If the list agrees
>> that running scan-build is a useful thing and that these problems should be fixed
>> then we could:
>>
>> (1) Add scan-build check to Travis CI but only print errors as warning
>> (2) Fix the 72 existing bugs over time
>> (3) Turn scan-build warnings into errors
>
> If they are warnings socked away in a Travis CI job that nobody looks
> out, then I doubt anybody is going to bother fixing them.
>
> Not that step (1) hurts necessarily, but I don't think it's really doing
> anything until step (2) is finished.
Agreed.
- Lars
next prev parent reply other threads:[~2017-02-25 23:11 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-24 17:29 [PATCH] travis-ci: run scan-build every time Samuel Lijin
2017-02-25 21:48 ` Lars Schneider
2017-02-25 22:31 ` Jeff King
2017-02-25 23:02 ` Lars Schneider [this message]
2017-02-26 2:09 ` Samuel Lijin
2017-02-26 14:12 ` Lars Schneider
2017-02-27 0:34 ` Samuel Lijin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=70DA368F-97FB-4492-811D-CCDF4F237939@gmail.com \
--to=larsxschneider@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=sxlijin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).