4

I was hit by a very stupid but hard to detect bug today. Here is the relevant code:

class Vector;
class PointIterator {
  const Vector & x;
  const Vector & yv;

  PointIterator(const Vector & xv, const Vector & yvo) : 
    x(xv), yv(yv) { ;};
  //          ^^ here is wrong
};

Why is such a code legal C++ ? Is there any situation where you could make use of the yv variable ? I'm aware of similar questions about int x = x+1;, (see this question) but while the latter isn't properly initialized, you still can use the x variable, while in the code above, I don't think you can make any use of yv.

Bonus point: is there any compilation option that would have made me detect this ? (preferably using gcc, but I also use clang), besides the "unused argument" warning (I have quite a few of those, I know I should clean them up).

Community
  • 1
  • 1
Vincent Fourmond
  • 3,038
  • 1
  • 22
  • 24
  • Do you mean **yv(yvo)** ???? instead of a recursive invocation ?? – ΦXocę 웃 Пepeúpa ツ Nov 19 '16 at 11:07
  • 1
    @ΦXocę웃Пepeúpaツ: Presumably the whole point of the question is that the OP does mean what he wrote. – Kerrek SB Nov 19 '16 at 11:10
  • good one.. should that be a typo OP??? – ΦXocę 웃 Пepeúpa ツ Nov 19 '16 at 11:11
  • 2
    This is where compiler warnings are your friend: `warning: reference 'yv' is not yet bound to a value when used here [-Wuninitialized]` (Apple LLVM version 8.0.0 (clang-800.0.42.1)) – johnsyweb Nov 19 '16 at 11:11
  • It's similar to `int x = x;` if you consider `int * p = p;`. A reference can be just as uninitialized as an object variable, with just as much UB. – Kerrek SB Nov 19 '16 at 11:12
  • @ΦXocę웃Пepeúpaツ Yeah, it was a not very clever variable name change. With yvo, everything works as expected, of course. – Vincent Fourmond Nov 19 '16 at 11:16
  • @KerrekSB But in int * p = p, you still can change p afterwards, which is not possible with a reference. – Vincent Fourmond Nov 19 '16 at 11:29
  • @VincentFourmond: well, true, kind of, but on the other hand, it may be UB right there, so in this other sense, `int * p = p;` can also never be used correctly and could have been just as forbidden. (But that depends on [how you squint at it](http://stackoverflow.com/q/14935722).) The reference example is certainly more curious. – Kerrek SB Nov 19 '16 at 11:40
  • Also see [Is passing a C++ object into its own constructor legal?](https://stackoverflow.com/q/32608458/1708801) – Shafik Yaghmour May 31 '17 at 23:05

1 Answers1

5

If you compile with g++ -O0 -g main.cpp -Wall -pedantic -Wextra -std=c++14 -o go

main.cpp: In constructor 'PointIterator::PointIterator(const Vector&, const Vector&)':
main.cpp:11:5: warning: 'PointIterator::yv' is initialized with itself     [-Winit-self]
 PointIterator(const Vector & xv, const Vector & yvo) :
 ^~~~~~~~~~~~~
main.cpp:11:53: warning: unused parameter 'yvo' [-Wunused-parameter]
 PointIterator(const Vector & xv, const Vector & yvo) :

As you can see, you get the warning two times. One for the self init and one for the unused parameter. Fine!

The same for clang:clang++ -O0 -g main.cpp -Wall -pedantic -Wextra -std=c++14 -o go

main.cpp:12:19: warning: reference 'yv' is not yet bound to a value when used
      here [-Wuninitialized]
        x(xv), yv(yv) { ;};
                  ^
main.cpp:11:53: warning: unused parameter 'yvo' [-Wunused-parameter]
    PointIterator(const Vector & xv, const Vector & yvo) : 
                                                    ^
main.cpp:8:20: warning: private field 'x' is not used [-Wunused-private-field]
    const Vector & x;

So clang reports also the problem, that the uninitialized ref is used before init. Fine!

What you learn: * use multiple compilers in highest warning level to get all warnings!

That is what we do for all our code, especially in unit tests connected to code coverage.

And you should use a coding guideline which makes it easy to detect such problems by review. Maybe use "m_" for class vars or "_var" for parameters or whatever you prefer. Var names with only a list of letters instead of speaking names is a not so well.

Klaus
  • 24,205
  • 7
  • 58
  • 113
  • This depends on the version of gcc. gcc 5.4 doesn't issue the init-self warning in this case. – Leon Nov 19 '16 at 11:16
  • @Leon Did you ask for it? – Deduplicator Nov 19 '16 at 11:16
  • @Deduplicator I just complemented the answer, by stating that it doesn't work for older versions of gcc – Leon Nov 19 '16 at 11:19
  • This does not show with my version of C++, gcc version 5.3.1 20160323, probably a sign I should upgrade... – Vincent Fourmond Nov 19 '16 at 11:23
  • @Leon: We actually have gcc 6.3 and 7.0 is coming soon. I did not know anyone who uses 5.x compiler any more. – Klaus Nov 19 '16 at 11:24
  • If I read the gcc output correctly, -Winit-self is the warning I'm after, thanks ! As for naming styles, I see the point now, it's just a little too late for a 60 thousand lines codebase ;-)... – Vincent Fourmond Nov 19 '16 at 11:25
  • Naming styles like "m_" or "_var" are not at all universal in C++. Generally, paying attention to compiler warnings completely eliminates the need to clutter source code with prefixes and suffixes like that. – Christian Hackl Nov 19 '16 at 11:26
  • @ChristianHackl: That is exactly why I wrote 'Maybe...' And having only warnings from one compiler is definitely not enough! – Klaus Nov 19 '16 at 11:27
  • @Klaus: Frankly, I see no problem when a parameter name is identical to a member name. That C++ allows this frees you from inventing artificially different names and from using prefixes and suffixes which tend to make code harder to read. At the end of the day, the OP's problem was the (self-admitted) fact that he was being sloppy about compiler warnings. – Christian Hackl Nov 19 '16 at 11:30
  • @ChristianHackl: " when a parameter name is identical to a member name". I see exact the problem which comes up in the op question. So our coding guidelines did not allow that and we check for that with static code analyser. But it is a question of style and every company can decide how to deal with such classic problems. – Klaus Nov 19 '16 at 11:32
  • @Klaus: As I said, the problem was not paying attention to compiler warnings. To say that every company can decide how to do it just evades the issue, because at one point *you* may be the one responsible for writing up the coding guidelines in your company! :) – Christian Hackl Nov 19 '16 at 11:33
  • For completeness, -Werror=init-self is the thing I was looking after. – Vincent Fourmond Nov 19 '16 at 11:35
  • @ChristianHackl: No, the problem was that OP did not switch on the warning which is quite different. And as Leon wrotes, older compilers did not have that warning. So I feel that good coding guidelines can help to prevent errors. Simply that. That all warnings/errors should be in place and also multiple compilers was already mentioned. – Klaus Nov 19 '16 at 11:38
  • @Klaus: Unused variables/arguments have been a pretty standard warning for decades in all compilers I've ever used. IMO, if you don't turn that one on, then all is lost anyway. – Christian Hackl Nov 19 '16 at 11:46