9

When I used the (recently released) Cppcheck 1.69 on my code1, it showed a whole lot of messages where I expected none. Disabling noExplicitConstructor proved that all of them were of exactly this kind.

But I found that I'm not the only one with a lot of new Cppcheck messages, look at the results of the analysis of LibreOffice (which I'm allowed to show in public):

screenshot of Cppcheck results on LibreOffice code

What would an experienced programmer do:

  • Suppress the check?
  • Massively introduce the explicit keyword?

1 This is of course not my code but code I have to work at work, it's legacy code: a mix of C and C++ in several (pre-)standard flavors (let's say C++98), and it's a pretty large code base.

Wolf
  • 9,679
  • 7
  • 62
  • 108
  • 7
    An experienced programmer would have used `explicit` where it is needed, and omitted it where implicit conversions are required. – juanchopanza May 05 '15 at 11:59
  • 15
    I can't speak for other programmers (experienced or otherwise), but I'd use `explicit` unless I specifically wanted to allow implicit conversions. It reduces the scope for surprises. – Mike Seymour May 05 '15 at 11:59
  • @juanchopanza I started using `explicit` in new code, but should I rework old code after this suggestion of a tool? – Wolf May 05 '15 at 12:02
  • 2
    cppcheck is just an advanced pattern matching tool. It's neither intelligent nor authorative, nor does it know anything about your intent. You should only take its output as a hint of something that might, possibly, under some circumstances, need fixing. An output with several thousand more or less identical items like this ones can usually be safely ignored as being "overzealous". I've written programs that generate tens of thousands false positives in cppcheck (of which exactly zero needed fixing). – Damon May 05 '15 at 12:03
  • @Damon this code I was checking is big and has definitely runtime errors, I'm always searching for hints to reduce errors that can be formalized in an obvious way, especially `explicit` could be interesting here. – Wolf May 05 '15 at 12:07
  • @MikeSeymour Absolutely, that's what I'd do with *new* projects. – Wolf May 05 '15 at 12:08
  • 1
    Hints can be a good thing, but don't be too anxious about anything that a tool (valgrind, cppcheck,...) tells you. Just try and run something like a wxWidgets (which, for the most part works without runtime errors) through cppcheck [did that with 1.68 a few months back] and you will get _thousands_ of "critical" errors which should cause runtime errors, such as returning pointers/references to temporaries. If you actually dig through the code, you find that they're false positives. Not saying that cppcheck _isn't_ of any help, just that you need to look at the output with a grain of salt. – Damon May 05 '15 at 12:12
  • 5
    What speaks *against* introducing explicit? (a) You touch many files, possibly making editing errors; (b) you have changes in many files which creates noise in comparisons between revisions. (c) The time and effort may be more effectively spent on targeted debugging. – Peter - Reinstate Monica May 05 '15 at 12:16
  • @PeterSchneider So you would suppress this check, how else can you cope with the "noise" Cppcheck is causing - couldn't this be also a bad advice? – Wolf May 05 '15 at 12:23
  • @Wolf I'd write a script to put explicit in the relevant places (based on the output of cppcheck), then see if anything fails to compile. – juanchopanza May 05 '15 at 12:30
  • 2
    No, not necessarily. I just tried to give considerations. The revision diff "noise" can be mitigated by (1) running all tests against the current revision; (2) doing nothing else but the "explicit" change first, and running the tests again, hopefully noticing any run time changes like new or disappeared bugs; and using that resulting revision as the defined base for all really substantial changes. I.e., do not go file by file and mix this with other bug fixes.(I think openssl went that path, cf. https://www.openssl.org/blog/blog/2015/01/05/source-code-reformat/.) – Peter - Reinstate Monica May 05 '15 at 12:33
  • Whether it's worth the effort I can't tell: it depends on how much work it is at all, how much you think -- based on samples -- errors are indeed caused by implicit conversions etc.; on the other side whether potentially more rewarding bug fixing strategies are available which should be pursued first. – Peter - Reinstate Monica May 05 '15 at 12:36
  • Alternatively, I require you to look at the PVS-Studio analyzer. For example, it showed good results with LibreOffice: http://www.viva64.com/en/b/0308/. Maybe it will be useful for your project. One of the advantages of this analyzer that it has large amount of settings, which allow to deal effectively with false positives. –  May 08 '15 at 06:13

1 Answers1

12

I've been bitten in the past by performance hits introduced by implicit conversions as well as outright bugs. So I tend to always use explicit for all constructors that I do not want to participate in implicit conversions so that the compiler can help me catch my errors - and I then try to always also add a "// implicit intended" comment to the ctors where I explicitly intend for them to be used as converting ctors implicitly. I find that this helps me write more correct code with fewer surprises.

… So I'd say "yes, go add explicit" - in the long run you'll be glad you did - that's what I did when I first learned about it, and I'm glad I did.

Jesper Juhl
  • 30,449
  • 3
  • 47
  • 70
  • 2
    Thanks for looking at this. BTW: cppcheck will still warn about it if you only use a comment. As to disable its warnings, I use inline-suppressions, so I have a functional comment about intentional implicit conversions. (my favourite answer so far ;-) ) – Wolf Feb 11 '16 at 12:18
  • An exception to this rule would be constexpr constructors. As these do not impose runtime overhead, they should be fine for implicit conversions. – EvertW May 26 '21 at 13:47