Thursday I interviewed with a company in the Houston, Texas area.

One of the interviewers had me examine two pieces of buggy code and try 
to identify the bugs.  I don't remember all of the details.  Here is the 
first one:

function() {
// some stuff

	if (whatever) {
		// code here to get a mutex lock
		// other stuff
	}
	// code here to release the mutex lock
	// the rest of the function code
}

The way I simplified it makes it easier to spot the bug.

The second one was issued with the warning that very few people can id 
the bug:

char* some_function() {
	char yada[100];

	char* p=yada;

	// some stuff that fills the array with data

	// and more stuff

	// and more stuff

	return p;
}

When I saw the return statement, and looked back to what the pointer was 
set to, I said, "I wouldn't do that."
"Do what?" the interviewer asked.
"Return a pointer to a local variable."
This provoked a laugh from the interviewer.

Seriously?  Not many people catch that?

Evidently not.  In chatting with the interviewers that day, they all 
confirmed that most bugs are stupid things like this.

Anyway, the job prospect is still a going affair, so I suppose I did 
okay in the interview.

Regards,
John

Post a reply to this message

On 04/03/2012 9:20 PM, John VanSickle wrote:
> Thursday I interviewed with a company in the Houston, Texas area.

Break a leg, John. :-D

-- 
Regards
     Stephen

Post a reply to this message

Nicely done!  I have to imagine that finding the interview questions to 
be too easy is certainly a good sign for your employment prospects. 
Come to think of it, as simple as that second question is it seems to be 
a pretty good way to test what level of "mental model" a person uses 
when coding, and since lots of people apparently botch it, it seems to 
be a pretty necessary test too.

Post a reply to this message

John VanSickle <evi### [at] kosherhotmailcom> wrote:
> Evidently not.  In chatting with the interviewers that day, they all 
> confirmed that most bugs are stupid things like this.

  Sadly, the vast majority of people programming in C (or C++) professionally
out there don't actually know the language enough to avoid such trivial ways
to shoot yourself in the foot. It may be something that you and me can spot
in a second, but I'm certain the average professional C programmer can't see
it. It's no wonder programs are so buggy. A sad state of affairs.

  In C++ in particular this is another situation were the majority of C++
programmers out there have no idea what's wrong:

//-------------------------------------------------------------
template<typename Value_t>
class Array
{
    Value_t* mData;
    std::size_t mSize;

 public:
    Array(std::size_t size):
        mData(new Value_t[size]),
        mSize(size)
    {}

    ~Array() { delete[] mData; }

    Value_t& operator[](std::size_t index)
    {
        assert(index < mSize);
        return mData[index];
    }

    const Value_t& operator[](std::size_t index) const
    {
        assert(index < mSize);
        return mData[index];
    }
};
//-------------------------------------------------------------

-- 
                                                          - Warp

Post a reply to this message

On 3/4/2012 2:33 PM, Warp wrote:
> John VanSickle<evi### [at] kosherhotmailcom>  wrote:
>> Evidently not.  In chatting with the interviewers that day, they all
>> confirmed that most bugs are stupid things like this.
>
>    Sadly, the vast majority of people programming in C (or C++) professionally
> out there don't actually know the language enough to avoid such trivial ways
> to shoot yourself in the foot.

It's quite a pity too, seeing as how C++ provides so many intricate and 
sophisticated ways to shoot yourself in the foot.

Post a reply to this message

On 03/04/2012 04:20 PM, John VanSickle wrote:
> Anyway, the job prospect is still a going affair, so I suppose I did
> okay in the interview.

Yep ... I'd say you did pretty darn good. Fingers crossed ... Keep us 
posted.

Post a reply to this message

Le 04/03/2012 23:33, Warp nous fit lire :
>   In C++ in particular this is another situation were the majority of C++
> programmers out there have no idea what's wrong:

* you used a template ? (nop, just a red herring for you! but a single
uppercase letter is usually preferred to Value_t)
* you have the [] checks the upper range (whereas usually it's at()
which check, and [] do not)
* you checked upper range, but not lower one
* you should have stated index in [] with const
* using assert is not the right way to handle the issue.
* you did not test in creator the value of size ( 0 ?)
* copy-constructor... you're in trouble with mData
* you did not disable automatic conversion of "number" into Array<>, so
if you have some different signature for some methods using either
number or Array<>, you're in for surprise (explicit)

With actual compiler:

* you forget the include needed for size_t (cstring)
* you forget the declaration of assert() (assert.h)

Post a reply to this message

>    In C++ in particular this is another situation were the majority 
of C++
 > programmers out there have no idea what's wrong:

Are you just referring to the lack of copy constructor and assignment 
operator, or is there something more subtle here?

  - Slime

Post a reply to this message

On 3/4/2012 14:33, Warp wrote:
>          return mData[index];

Given the previous discussion, I'm betting this is the error Warp was 
talking about - returning the address of a variable that gets disposed when 
this object gets disposed. It's basically the same bug - scopes exceeding 
lifetimes.  I'm amused that Ada actually prevents this bug unless you 
explicitly say "Yes, yes, I'm intentionally assigning a value to a pointer 
whose lifetime is shorter than that of the pointer."

-- 
Darren New, San Diego CA, USA (PST)
   People tell me I am the counter-example.

Post a reply to this message

Le 05/03/2012 01:10, Darren New a écrit :
> On 3/4/2012 14:33, Warp wrote:
>>          return mData[index];
> 
> Given the previous discussion, I'm betting this is the error Warp was
> talking about - returning the address of a variable that gets disposed
> when this object gets disposed. It's basically the same bug - scopes
> exceeding lifetimes.  I'm amused that Ada actually prevents this bug
> unless you explicitly say "Yes, yes, I'm intentionally assigning a value
> to a pointer whose lifetime is shorter than that of the pointer."
> 

I doubt it a bit. It would be at worst an issue with the
copy-constructor of Value_t (if any), but I would think (maybe wrongly)
that due to the prototype of the function (returning a reference to
Value_t), the mData[index] is actually the real reference (compiler
handling the issue of &/* for us here).

One real bug is when used with the following code:

int foobar()
{
 Array<int> foo(3);
 foo[0] = 3;
 foo[1] = 7;
 foo[2] = 0;
 {
 Array<int> bar(foo); // get a copy of foo
 bar[2] = bar[1] + bar[0]; // no problem, it's 3+7
 }
 foo[2] += foo[1] * foo[0];     // you might crash here already
 return foo[2];     // these result is going to be surprising sometime
}                   // you definitely have a problem with glibc here!

Same kind of issue might happen if you provide Array<> to methods by
values without const (hence needed some copy-constructor... and the call
to the destructor for the intermediate objects)

-- 
A good Manager will take you
through the forest, no mater what.
A Leader will take time to climb on a
Tree and say 'This is the wrong forest'.

Post a reply to this message

Slime <pov### [at] slimelandcom> wrote:
>  >    In C++ in particular this is another situation were the majority 
> of C++
>  > programmers out there have no idea what's wrong:

> Are you just referring to the lack of copy constructor and assignment 
> operator, or is there something more subtle here?

  Since I was talking about completely trivial errors that the majority
of C++ programmers can't see, what do you think?

-- 
                                                          - Warp

Post a reply to this message

Le_Forgeron <jgr### [at] freefr> wrote:
> * you used a template ? (nop, just a red herring for you! but a single
> uppercase letter is usually preferred to Value_t)

  Preferred by who? A single uppercase letter is only obfuscation.

> * you have the [] checks the upper range (whereas usually it's at()
> which check, and [] do not)

  I'm not creating a C++ standard container. If the class specs say that
an assertion failure will happen on an invalid index, then it's ok.

> * you checked upper range, but not lower one

  Why would I check a lower range when std::size_t is by definition
unsigned? ("index >= 0" is always true.)

> * you should have stated index in [] with const

  Why? It doesn't make any difference from the outside, and the
implementation is trivial.

> * using assert is not the right way to handle the issue.

  It depends on how you spec the class. It's certainly the *easiest* way
of doing it.

> * you did not test in creator the value of size ( 0 ?)

  "new Value_t[0]" is perfectly valid C++ and will not cause malfunction.

> * copy-constructor... you're in trouble with mData

  That's the problem with the class.

> * you did not disable automatic conversion of "number" into Array<>, so
> if you have some different signature for some methods using either
> number or Array<>, you're in for surprise (explicit)

  Maybe.

> * you forget the include needed for size_t (cstring)
> * you forget the declaration of assert() (assert.h)

  I skipped #include lines for brevity. (Besides, std::size_t is officially
declared in <cstddef>.)

-- 
                                                          - Warp

Post a reply to this message

Darren New <dne### [at] sanrrcom> wrote:
> On 3/4/2012 14:33, Warp wrote:
> >          return mData[index];

> Given the previous discussion, I'm betting this is the error Warp was 
> talking about - returning the address of a variable that gets disposed when 
> this object gets disposed.

  No. It's not returning a reference to a local variable. Returning a
reference to a member variable is standard practice (for example all
standard data containers do that.) You wouldn't be able to modify the
members of the array using operator [] from the outside otherwise.
(In the const version of the operator it would be possible to return
the element by value. However, this is not usually done because the
members might be heavy to copy.)

  (Yes, it would be possible for the calling code to define a reference
or pointer to the array element and keep it for longer than the lifetime
of the array. However, that kind of error is no different from doing it
with raw arrays or any other type of data container. A class acting as
a wrapper for a data container cannot guard the calling code from this.)

-- 
                                                          - Warp

Post a reply to this message

Le_Forgeron <lef### [at] freefr> wrote:
> void foobar()
> {
>  Array<int> foo(3);
>  Array<int> bar(foo); // get a copy of foo
> }                   // you definitely have a problem with glibc here!

  You don't need anything more than that.

-- 
                                                          - Warp

Post a reply to this message

Warp escreveu:
> John VanSickle <evi### [at] kosherhotmailcom> wrote:
>> Evidently not.  In chatting with the interviewers that day, they all 
>> confirmed that most bugs are stupid things like this.
> 
>   Sadly, the vast majority of people programming in C (or C++) professionally
> out there don't actually know the language enough to avoid such trivial ways
> to shoot yourself in the foot. It may be something that you and me can spot
> in a second, but I'm certain the average professional C programmer can't see
> it. It's no wonder programs are so buggy. A sad state of affairs.
> 
>   In C++ in particular this is another situation were the majority of C++
> programmers out there have no idea what's wrong:
> 
> //-------------------------------------------------------------
> template<typename Value_t>
> class Array
> {
>     Value_t* mData;
>     std::size_t mSize;
> 
>  public:
>     Array(std::size_t size):
>         mData(new Value_t[size]),
>         mSize(size)
>     {}
> 
>     ~Array() { delete[] mData; }
> 
>     Value_t& operator[](std::size_t index)
>     {
>         assert(index < mSize);
>         return mData[index];
>     }
> 
>     const Value_t& operator[](std::size_t index) const
>     {
>         assert(index < mSize);
>         return mData[index];
>     }
> };

the C issues were self-evident, but this is not much easier than trying 
to read some idiomatic haskell... :P

BTW, why

      Array(std::size_t size):
          mData(new Value_t[size]),
          mSize(size)
      {}

rather than

      Array(std::size_t size)
      {
          mData=new Value_t[size];
          mSize=size;
      }

when you've got exactly 2 extra chars (:,) in the first case?  Don't 
tell me it's because some compile-time initialization that is not done 
in the second case...

is also delete[] defined when you define those operator[] methods?

C++ is so alien...

-- 
a game sig: http://tinyurl.com/d3rxz9

Post a reply to this message

On 3/5/2012 11:13 AM, nemesis wrote:
> BTW, why
>
> Array(std::size_t size):
> mData(new Value_t[size]),
> mSize(size)
> {}
>
> rather than
>
> Array(std::size_t size)
> {
> mData=new Value_t[size];
> mSize=size;
> }
>
> when you've got exactly 2 extra chars (:,) in the first case? Don't tell
> me it's because some compile-time initialization that is not done in the
> second case...

It's because the second case doesn't initialize the variables, but 
rather assigns to them.  This distinction matters in C++, and in general 
here's how the code plays out in the two cases when you instantiate the 
class:

case 1:
1 - call mData constructor with parameter `new Value_t[size]'
2 - call mSize constructor with parameter `size'

case2:
1 - call no-parameter mData constructor
2 - call no-parameter mSize constructor
3 - call mData assignment operator with parameter `new Value_t[size]'
4 - call mSize assignment operator with parameter `size'

In this particular case I think the code generated by most compilers 
would be identical, but the former is better practice since in other 
situations the generated code will be different.  There's also cases 
where the second case won't even compile, for instance if one of the 
member fields lacks a zero-parameter constructor.

> is also delete[] defined when you define those operator[] methods?

delete[] is built in to the language and frees the memory associated 
with a call to an array allocation performed with new[].

> C++ is so alien...

Depends on what languages you feel at home in, obviously.

Post a reply to this message

nemesis <nam### [at] gmailcom> wrote:
> BTW, why

>       Array(std::size_t size):
>           mData(new Value_t[size]),
>           mSize(size)
>       {}

> rather than

>       Array(std::size_t size)
>       {
>           mData=new Value_t[size];
>           mSize=size;
>       }

> when you've got exactly 2 extra chars (:,) in the first case?  Don't 
> tell me it's because some compile-time initialization that is not done 
> in the second case...

  Using the initialization list is recommended and, in some cases,
actually the only possible way of initializing certain members.
Usually you write actual code in the constructor only if a member
cannot be constructed in the initialization list. (In this particular
example there's no discernible benefit, except perhaps for a consistent
style of initialization.)

  Examples of members that can only be initialized in the initialization
list are const members, references and objects with no default constructor.
Also, even if the object had a default constructor but you wanted to
initialize with a different constructor, the only way to do that is to
do it in the initialization list.

//-------------------------------------------------------------------
class ClassWithoutDefaultConstructor
{
 public:
    ClassWithoutDefaultConstructor(int);
};

class ClassWithAdditionalConstructors
{
 public:
    ClassWithAdditionalConstructors();
    ClassWithAdditionalConstructors(int);
};

class Example
{
    int& referenceMember;
    const std::string constMember;
    ClassWithoutDefaultConstructor obj1;
    ClassWithAdditionalConstructors obj2;

 public:
    Example(int& aReference):
        referenceMember(aReference),
        constMember("Hello"),
        obj1(5),
        obj2(10)
    {
        // none of the above can be initialized here
    }
};
//-------------------------------------------------------------------

> is also delete[] defined when you define those operator[] methods?

  operator[] has nothing to do with new[] and delete[].

-- 
                                                          - Warp

Post a reply to this message

On 3/5/2012 12:05, Warp wrote:
>>            mData(new Value_t[size]),

> example there's no discernible benefit, except perhaps for a consistent
> style of initialization.)

WOuldn't it also be safer if that 'new' call threw an exception?

-- 
Darren New, San Diego CA, USA (PST)
   People tell me I am the counter-example.

Post a reply to this message