29

This one made me think:

class X;

void foo(X* p)
{
    delete p;
}

How can we possibly delete p if we do not even know whether X has visible destructor? g++ 4.5.1 gives three warnings:

warning: possible problem detected in invocation of delete operator:
warning: 'p' has incomplete type
warning: forward declaration of 'struct X'

And then it says:

note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined.

Wow... are compilers required to diagnose this situation like g++ does? Or is it undefined behavior?

Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
fredoverflow
  • 256,549
  • 94
  • 388
  • 662

3 Answers3

22

From the standard [expr.delete]:

If the object being deleted has incomplete class type at the point of deletion and the complete class has a non-trivial destructor or a deallocation function, the behavior is undefined.

So, it's UB if there's nontrivial stuff to do, and it's ok if there isn't. Warnings aren't neccessary for UB.

lrineau
  • 6,036
  • 3
  • 34
  • 47
etarion
  • 16,935
  • 4
  • 43
  • 66
  • 3
    No, it's not "UB if", it's unconditional UB. For example that class could have `operator new` overloaded on a separate heap and `delete` statement will now call wrong `operator delete`. – sharptooth Dec 01 '10 at 14:17
  • 6
    I stroked out the ambiguous part. As far as I can see, the standard doesn't say that delete-ing objects of incomplete type is UB in every case, just as mentioned in the section that I quoted. Why do you think that it is UB unconditionally? (Where does the standard say this?) – etarion Dec 01 '10 at 14:23
  • 1
    @etarion: The standard says that behavior will depend on how that class is declared which means that you can start with a class satisfying those requirements and then change it to not satisfy those requirements and now you face UB (which can be "it works" by the way). So although formally you're clean you've planted a fatal error into your code. The warning in question should be addressed - `delete`ing incomplete classes is a very bad idea. – sharptooth Dec 01 '10 at 14:33
  • 1
    So it IS actually conditional UB, and not unconditional as you claimed. (I'm not arguing if it's good practice or not. The question in the OP was if a diagnosis is required - no - and if it's UB - that depends.) – etarion Dec 01 '10 at 14:34
  • 1
    I will +1 this answer if you un-cross-out the "nontrivial" bit. It's exactly right - you can delete "something" of incomplete type provided that whoever eventually completes that type ensures it has trivial destruction and deallocation. Hopefully this will be documented somewhere, or perhaps that "whoever" is you. It's no more "unconditional UB" that it is "unconditional UB" to pass a null pointer into a forward-declared function. After all, the function *could* dereference it. But if the docs say it doesn't, you're fine. Or at least, it's someone else's fault if it does... – Steve Jessop Dec 01 '10 at 16:00
  • Hmm, I'm not sure about null pointers. The text assumes that an object is being deleted - this is not the case if you pass a null pointer. So it might still be undefined behavior because of a lack of saying what happens with null pointers. However it has the same lack in other texts that clearly shouldn't yield to UB. So I'm undecided. – Johannes Schaub - litb Dec 01 '10 at 16:42
  • @Johannes: I didn't mean both things at once, I was just comparing, `void foo(char*); foo(0);`. This isn't "unconditional UB" (sharptooth's terminology) just because foo *might* be defined as `void foo(char*p) {*p;}`. Likewise, deleting through an incomplete type isn't "unconditional UB" just because the type *might* be completed with a non-trivial destructor. – Steve Jessop Dec 01 '10 at 18:02
  • @Steve I understand. It wasn't meant to be a reference to your comment :) I think I should more often include warnings such as "not related to statement of XYZ" since I find people make more associations than I would expect. My bad. – Johannes Schaub - litb Dec 01 '10 at 18:12
  • @Johannes: Oh good. I think 5.3.5/2 covers null pointers in a way that isn't/can't be contradicted by 5.3.5/5. There is no "object being deleted", because the operation "has no effect" and so in particular it doesn't delete an object. Hence the "if" is false. – Steve Jessop Dec 01 '10 at 18:45
  • @Steve well it doesn't say "If an object being deleted has ..." but it says "If the object being deleted has ...". If it would say the former, I could see it doesn't apply to null pointers. 5.3.5/2 saying that deleting a null pointer "has no effect" doesn't really mean much, since an implementation is still allowed to call an deallocation function with a null pointer argument. I guess it's fine to delete null pointers though, but I really wouldn't want to bet. – Johannes Schaub - litb Dec 01 '10 at 18:47
  • @Johannes: Fair point, but I don't see how 5.3.5/5 can conjure into existence the referand of a null pointer ;-) /2 also says, "In the second alternative (delete array), the value of the operand of delete shall be the pointer value which resulted from a previous array new-expression. If not, the behavior is undefined.". I think it's pretty clear that this is not meant to contradict the earlier statement that a null pointer is defined as a no-op, and the same can be said for the rest of the description of delete expressions. – Steve Jessop Dec 01 '10 at 18:50
  • @Steve I agree with that. However, C++0x's text changed here. At /2 it now says explicitly "If it is not a null pointer, in the first alternative ...", and /6 says explicitly "If the value ... is not a null pointer", while /5 still doesn't say so. So the intent seems to be towards UBness, but then /3 also omits it, so I've no clue what they want it to be :) – Johannes Schaub - litb Dec 01 '10 at 18:55
  • @Johannes: interesting. I wonder whether they decided to omit it (and hence AFAIK introduce for the first time the concept of a null pointer referring to "an object"), or just added some tightening language to avoid any possibility of a contradiction in those other clauses, and didn't think it worth mentioning in /5. In /3, I don't think null pointers have a dynamic type, so just like "the object being deleted", the phrase "its dynamic type" is talking about something that doesn't exist. Should we assume that, like NaN, non-existent things don't compare equal to anything? ;-) – Steve Jessop Dec 01 '10 at 18:58
  • @Steve http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#348 uncovers that the change has nothing to do with our concerns -.- – Johannes Schaub - litb Dec 02 '10 at 00:21
  • Also 12.5/4 that actually defines what deallocation function is called has no handling of null pointers. So it appears to be unclear what happens for a null pointer. However it *appears* that the last sentence of it applies: "If this lookup fails to find the name". That sentence will definitely apply for any T that isn't of class type. Maybe it is also intended to apply for null pointers. :) – Johannes Schaub - litb Dec 02 '10 at 00:36
7

It is undefined behavior.

However, you can make the compiler check for incomplete types, like boost:

// verify that types are complete for increased safety

template<class T> inline void checked_delete(T * x)
{
    // intentionally complex - simplification causes regressions
    typedef char type_must_be_complete[ sizeof(T)? 1: -1 ];
    (void) sizeof(type_must_be_complete);
    delete x;
}

Applying sizeof to an incomplete type should trigger an error, and I suppose if that passes with some compiler, then an array of negative size would trigger an error.

UncleBens
  • 40,819
  • 6
  • 57
  • 90
4

It is undefined behaviour, and a common gotcha when implementing the pImpl pattern. To the best of my knowledge, there is simply no such thing as a warning that the compiler is required to emit. Warnings are elective; they're there because the compiler writer thought they would be useful.

Karl Knechtel
  • 62,466
  • 11
  • 102
  • 153