C++ Question

Hardware and technical stuff
1

EDIT: I originally had two (unintentional) do loops, with shadowed index variables iCurrWin. This was the problem. Why it worked in one code set and the bottom isn’t clear, but it’s not something you’d ever intentionally do anyway so it’s not important. See below for a discussion on the merits of braces and how my usage is apparently nonstandard, though, if you like. And thanks for all the suggestions.

So, I ran into something today while writing a program that not only did I not understand, I still don’t. My code originally was this:


  vector<Mat3> M1(iWindows,Mat3::ZERO),M2(iWindows,Mat3::ZERO);
  vector<Mat3> M3(iWindows,Mat3::ZERO),M4(iWindows,Mat3::ZERO);

  for (int iCurrWin=0; iCurrWin < iWindows; ++iCurrWin)
  {
    for (size_t iRow=0; iRow < 3; ++iRow)
      for (size_t iCol=0; iCol < 3; ++iCol)
      {
       //define cInd1, cInd2, rInd1, rInd2
          (M1[iCurrWin])(iRow,iCol)= t3_S[iCurrWin](iRow,iRow,iCol,iCol);
          (M2[iCurrWin])(iRow,iCol)= t3_S[iCurrWin](iRow,iRow,cInd1,cInd2)
                                    +t3_S[iCurrWin](iRow,iRow,cInd2,cInd1);
          (M3[iCurrWin])(iRow,iCol)= t3_S[iCurrWin](rInd1,rInd2,iCol,iCol);
          (M4[iCurrWin])(iRow,iCol)= t3_S[iCurrWin](rInd1,rInd2,cInd1,cInd2)
                                    +t3_S[iCurrWin](rInd1,rInd2,cInd2,cInd1);
      }
    // define eToS
    M1[iCurrWin]*=eToS;
    M2[iCurrWin]*=eToS;
    M3[iCurrWin]*=2.0*eToS;
    M4[iCurrWin]*=2.0*eToS;
  }

I have now changed it to this:


  vector<Mat3> M1,M2,M3,M4;

    for (int iCurrWin=0; iCurrWin < iWindows; ++iCurrWin)
    {
      Mat3 m3_LM1, m3_LM2, m3_LM3, m3_LM4;
      for (size_t iRow=0; iRow < 3; ++iRow)
        for (size_t iCol=0; iCol < 3; ++iCol)
        {
          //define cInd1, cInd2, rInd1, rInd2
          m3_LM1(iRow,iCol) = t3_S[iCurrWin](iRow,iRow,iCol,iCol);
          m3_LM2(iRow,iCol) = t3_S[iCurrWin](iRow,iRow,cInd1,cInd2)
                                     +t3_S[iCurrWin](iRow,iRow,cInd2,cInd1);
          m3_LM3(iRow,iCol) = t3_S[iCurrWin](rInd1,rInd2,iCol,iCol);
          m3_LM4(iRow,iCol) = t3_S[iCurrWin](rInd1,rInd2,cInd1,cInd2)
                                     +t3_S[iCurrWin](rInd1,rInd2,cInd2,cInd1);
        }
    //define eToS
    M1.push_back(m3_LM1*eToS);
    M2.push_back(m3_LM2*eToS);
    M3.push_back(m3_LM3*2.0*eToS);
    M4.push_back(m3_LM4*2.0*eToS);
    }

There are no function calls here; the things that look like function calls are overloaded indexing operators to index into a matrix or a 4th order tensor. The other overloaded operators have been independently verified to work properly. Everything works fine if iWindows == 1.

The latter code works fine for arbitrary iWindows; the former does not. When I run through the loop in the former, initial assignment works fine if I dump the values directly to a file, but on inserting a check loop directly after the code to simply re-output the vectors, all but the last are screwed. This smells like I’m overwriting memory somehow, but I don’t see how.

Can anyone tell me what I did wrong?

2

I tried to read that code, but when you’ve got loop counters with the same name in nested loops…yeah, that’s going to make deciphering shit pretty arduous.

Not trying to be snarky, but seriously, shadowed loop counter names? Nothing but bad things can happen from that.

3

You’re using iCurrWin as the counter variable in a loop nested in another loop with iCurrWin? That’s just… wow.

4

Heh… that’s what happens when you look at something too long. Didn’t even notice that the first time around. Definitely not good there. Let me clean that up and re-test it. Apparently I need to change some warning setting in g++ as well, since I could have sworn g++ used to warn me about shadowing declarations, and I wouldn’t have intentionally written that code nor ignored a warning.

5

I’d guess that Mat3::operator *=() isn’t putting the right value back into the array. Is it an in-place modify or does it return a new Mat3?

Edit: Or, problem was solved already. Nevermind.

6

Yeah, that wasn’t intentional. I’ve updated the code above, and retested results, and they still differ and I don’t know why. It should be a matter of in the first I create the vector with zero-defined matrices, in the second I create the matrix and append to the vector, and they both should logically work the same way, yet they don’t.

Edit: And on further investigation when I remove the shadowed counter variable mess they work the same. It’s odd that one works okay despite that and the other doesn’t, but considering that should never have been there in the first place I’m not going to worry about it. I will worry about finding better warning generation from g++'s compiler so that it catches stuff like that next time.

Thanks for pointing that out guys.

7

Where does the Mat3 class come from? I suspect the problem lies in it, or in the way you arw using it.

8

  1. assert() is your friend. Litter your shit with invariant checks, pre/post-conditions, etc. and you can usually spot this stuff fast.

  2. Dude, more braces. You have two for loops with braces and one without and it’s trivial for bugs to creep in like that.

9

How would I use assert to catch the shadowing of a counter variable that in this case was causing the problem?

I disagree about braces; I explicitly excluded braces on the middle loop to make it clear that it simply is an intermediate index counter to propel the inner loop. Nothing should be done at the middle loop level for this logic, so there are no braces which might inadvertantly catch stray instructions. I do a lot of index contraction in the code I write (matrix/tensor operations) so this is a fairly standard convention for me.

10

I always, always, always use braces, even if the if/for/whatever is a single line of code. Because maybe later I’ll add more code and forget to add the braces if I’m just quickly buzzing through the code, and then there will be an error that the compiler probably won’t catch.

11

Sorry, I didn’t mean in this case, I’m saying as a general debugging strategy. The earlier some expected condition fails, the faster you can find the problem (assuming it’s a logical problem). Divide by zero, out of range stuff, etc.

I disagree about braces; I explicitly excluded braces on the middle loop to make it clear that it simply is an intermediate index counter to propel the inner loop. Nothing should be done at the middle loop level for this logic, so there are no braces which might inadvertantly catch stray instructions. I do a lot of index contraction in the code I write (matrix/tensor operations) so this is a fairly standard convention for me.

I guess I’ll just disagree as well and leave it that – matching braces is absolutely vital for me no matter where it is because I have seen bugs appear when additional code is added and it looks like it’s bound to one loop or branch and it’s not…

12

I agree with more braces. Totally.

Also, ditch all the i and r and c prefixes. They only make it more sluggish for your brain to manipulate what’s on the screen.

Using size_t to index arrays seems weird but okay, whatever.

I never use the STL, so I honestly don’t know the answer to this question, but if you assign to an empty slot in a vector with =, it is not going to auto-enlarge its size to include that, right? That would be weird behavior. So I would guess that this code doesn’t work because at the end of things, each vector still thinks it has 0 items in it, despite the fact that you have initialized the memory.

13

As for the this…without a debugger, it’s hard to say since we can’t see what’s going on inside those various overloaded operators. It could be something to do with memory, like in some conditions heap is used instead of stack or whatever, etc.

Also, I’m still confused – after the changes, which one works, and which doesn’t, and how is the error manifested?

14

The thing about always using braces is that it lets your brain rest a little more than it otherwise would. You don’t have to have some tiny subconscious question about whether not having a brace there is going to do anything. In fact, when you are writing code, you don’t have to have that moment of consideration about whether to put the brace there or not. You just put the brace in.

I find this to be worth a lot in terms of being able to get stuff done, but I suppose everyone is different.

15

I always use braces.

In this case, with all these manipulation of an array with parameters, I would actually break it out a bit with a few references so I could figure out what the hell was going on.

16

After the changes both work. Before, the shadowed pre-populated vector didn’t, but the push_back to the vector did. I’ll edit the post so that folks don’t try to answer a question that’s not one any more. (I forget how many programmy types are around that might be genuinely curious sometimes, sorry.)

17

I’m with all those voting for the extra braces. I had to look very carefully at the second example’s push_back() lines to see which loop they belonged to. That wouldn’t have been an issue with the braceless for loop. I’ve had issues with being unable to set proper breakpoints near braceless for loops as well.

18

It’s an STLism – the container size() method returns a size_t, so if you want your compiler to shut up about signed vs. unsigned compares, you either cast a lot or you capitulate and use a size_t as your loop index.

A part of me has died for simply knowing this. Fucking C++ and STL…

19

Boring shit:
Whether you use semantic prefixes like i, c, and whatnot is a judgement call based on how good your coding tools are. Personally I find it Visual Studio reading of C++ files with reasonable naming I don’t need them, but whatever.

More importantly, the names should be clear to someone who knows your field but didn’t write the code. What’s eToS and cInd, etc?

More useful: if you find a way of encapsulating the inner operation you can probably also find a way to do so for the entire operation, preventing things like indexing messes. Also, isn’t there standard libraries for vector stuff?

20

Egads that code is… opaque. I’m so glad I don’t have to write C++ anymore.