Superstitious Code
Superstitious Code
for ($i = 0; $i < 100; $i++) {
next if ($i == 13);
....
}
if (this == null)
{
// Logic here
}
Code written by some kind of BadProgrammer, or really exhausted adventurous programmer.
A lot of superstitious code I've seen was just intended to solve a bug, but was just a workaround that didn't solve the real problem. -- ChristopheThibaut
Couldn't this also be a temporary disabling structure that is accidentally left in like this is: #if 0
//code#endif but more inefficient? -- ELangley See also VoodooChickenCoding, TrialAndErrorProgramming, WishfulCoding
In Java I often see this:
try {
// database access code
} catch (Exception ex) {
// handle error
dbConnection.close(); // Cleanup code here is purely superstitious
} finally {
dbConnection.close();
}
Superstitious code can be written in ignorance, but to me this is just ignorant code, not superstitious code. OTOH, if there's a dbConnection.close() in the try block....
Is this superstition, or healthy paranoia?
class Foo {
private void *my_magic;
private static void *const class_magic;
// Constructor
Foo() {
make_stuff();
my_magic = class_magic;
}
void is_magic() {
if (my_magic != class_magic)
throw ProgrammerScrewedUpAgainException?;
}
// Destructor
~Foo() {
is_magic();
delete_stuff();
my_magic = 0;
}
// Superstition or paranoia?
void method wibble() {
is_magic();
do_wibble_here();
}
// Controversy
void combined_wibble(Foo &bar) {
is_magic();
bar.is_magic();
do_binary_wibble(bar);
}
// So controversial, we might all agree it's bad ;-)
void familiar_wibble(Quux &bletch) {
is_magic();
bletch.is_magic();
do_heterogeneous_wibble(bletch);
}
};
// Guaranteed to be unique (we think ;-)
Foo::class_magic = &Foo::class_magic;
In C++, I have done things similar myself, because there is an unidentified memory overwriting bug somewhere in the program, this kind of superstitious checking is the only way to guard against it. Otherwise, you will be spenting a lot of time handling bug reports that said the stack trace of a core dump points to your class causing segmentation faults, which is actually due to someone else's code messed up your my_magic pointer.
A justifiable? case.
I did this, albeit in C++.
void Component::destroy() {
if( 0 == this ) return; // base case
... recursively destroy rest of component tree ...
}
It was justified for various reasons. If I was more magical, I would have removed the recursion, making the check nearly useless, but it would still have to remain to avoid the ugly if( pComponent ) pComponent->destroy(). And before you complain, I was in the process of removing all the constructors and destructors, so no donut there. -- SunirShah
Let the argument begin...
NOTE: AnonymousDonors may not be related.
(I've reorganized the below and altered some statements I've made. This may have invalidated some arguments or cast some discussions in my favour when they were not previously. I preferred to be clearer than preserve the discussion as it stood. -- ss)
Language law
Coding to protect yourself against those instances where undefined behavior (dereferencing a null pointer) happens to behave as you anticipated? That looks to me like a Bad Idea Tm. What aren't you telling us here, Sunir - does your implementation offer guarantees about the behavior here? As written, it isn't portable, and I would certainly hope that the real life instance of this code confirms the host implementation during the compile phase. -- DanilSuits
It's not undefined. You don't have to dereference the instance pointer to make a statically bound call. The behaviour fully defined in fact (see below). However, I'm open to the fact that it won't work at some point. Fix it then. However, if I was still working on that contract, my "fix" would have continued the progression towards C. Then it would become Component::destroy( pComponent ). Don't forget that pInstance->method() is just syntactic sugar for Class::method( pInstance ) unless method() is virtual. No constructors, no virtuals. -- SunirShah
5.2.5/2 and 5.2.5/3 seem to be the most obviously relevant bits of the standard, in particular that E1->E2 is converted to the equivalent (*(E1)).E2 and yes, the first part of that is evaluated. I don't believe you can manage that without dereferencing the null pointer... -- DanilSuits
You can evaluate the expression without dereferencing the pointer. There are multiple evaluation phases defined in C++. *(E1) may be evaluated statically at compile time within the type system of the program, well before runtime. This is fully within the expectation of the language design. To be more careful with the standard, when the semantics of one expression is described as an isomorphism of another, you have to continue reading the semantics of the second to understand what's intended. Here's the appropriate language lawyering (in my reading).
In an earlier draft version, 5.2.5/1+footnote5 actually describes how the expression before the dot or arrow is evaluated regardless of necessity, which is more to your point. However, this doesn't mean what you think it does.
5.2.2 expr.call gives us this (bold mine):
I think you'll find that, even if the above interpretation were correct, you'd still run afoul of 9.3.1 If a nonstatic member function of a class X is called for an object that is not of type X, or of a type derived from X, the behavior is undefined. As I understand it, the thing on the other side of a null pointer of type X is not an object of type X. -- DanilSuits
pComponent is not a typeless null pointer (i.e. not a constant-value expression evaluating to (void*)0), but a pointer to type Component, making the static type clear and obvious. Consider that in your argument, we would have to wait to runtime to determine the static type, as it is not clear what the value of pComponent is at compile time. Even if Component* const pComponent = 0; 4.10.1 describes the appropriate null pointer conversions. A constant 0 can become any pointer, including a pointer to function or pointer to member function, or in this case, a pointer to an object type. Don't forget that C++ is a strongly typed language. If 0 was not a subtype of T *, it would be impossible to compile anything requiring an cast from 0 to T*, such as a variable assignment or a function parameter binding, without an explicit cast. Indeed, this is the problem with NULL, which is #define NULL ((void*)0) typically, and void* does not automatically convert to every type (in particular, pointers to functions and member functions), unlike 0. -- SunirShah
Of course, but apple juice. The static types of the elements of the expression have nothing to do with (this==0). How are you supposed to have reached such a state without invoking undefined behavior? You can't have dereferenced a null pointer (1.9/4), which means you can't have used indirection (index), so you can't have used unary * (5.3.1/1), so you can't have used the -> operator (5.2.5/3). And even if you could somehow manage to bypass all of that, 9.3.1 says you have to have an object. Yes, an implementation could invoke the function as you expect it to, there is sufficient information present. It could also launch Quake without sacrificing any conformance at all.
Apparently you did not understand what I quoted, and this has lead you to be snippy and sarcastic.
I don't see how it could be any clearer. Component::destroy() is non-virtual. The object-expression after equivalence from pComponent->destroy() to (*pComponent).destroy() is (*pComponent). The type of pComponent is Component*, so the type of the object-expression is Component. The method destroy() is selected from Component and then invoked statically liked a normal function call. Your insistence that the value of pComponent is important to this invocation doesn't make any sense because the value is not known at compile time. Further, a static invocation doesn't dereference the object pointer ever for anything. -- SunirShah
I agree, I don't see how it could be any clearer either. Component::destroy() is a non-static [remember, static is overloaded in c++]member function. 9.3.1 requires that calling a non-static member function requires (1) an object and (2) that the object be of the correct type. "this" is the address of that object, by 9.3.2. We agree that, if there is an object, that it will be of the correct type, but I don't see anything that suggests there is an object. We agree that we have a null pointer of type Component, but I don't see any justification for assuming there is an object associated with that pointer. So the behavior is undefined. Certainly the static analysis you suggest makes it clear which function is intended, by under the hypothetical conditions where this==0 could be true, a conforming implementation is not required to call that function. -- DanilSuits
You claim that because it is a null pointer, there is no object, and therefore no valid call. I don't think that's consistent with the rest of the specification, especially the definition of static-type in 1.4 I think you mean 1.3.11 [defns.static.type] -- dps
-- static type: The static type of an expression is the type (_basic.types_) resulting from analysis of the program without con- sideration of execution semantics. It depends only on the form of the program and does not change while the program is executing.After all, if the program has undefined behaviour depending on the runtime value of pComponent, then the static type lookup required by a nonvirtual member function invocation would be invalidated, which is illegal by the definition of static type. That's where we'll have to leave it, I think. Agree to disagree. (Would you care to clean up any of the vitriol?) -- SunirShah
I'd just get rid of all of it - what would you like to keep? -- DanilSuits
All the text that describes the situation from the perspective of the specification; I'm leaving the country for several months so I won't be here to effect the changes. I think there's value in the discussion. If you clean it up, I trust you to do it fairly. -- SunirShah
In practical terms, there is not much to argue anyway. As I said, even if the team encounters a strange compiler that did a 0-value check before each invocation (some did exist a long time ago during the experimental C++ compiler period), the solution is just to convert the expressions into the equivalent C calls as is the ultimate intent anyway a la cfront. -- SunirShah
Virtual "What if?"
NOTE: In the case cited above, virtuals can't exist as there are no constructors allowed. If they were allowed, problems do exist. This section describes those.
Any component that does
void Component::Foo() {
if( 0 == this ) ....
else ....
}
is looking for serious trouble one day. Someone somewhere in a base class of Component makes Foo virtual and boom. -- AnonymousDonor
You're absolutely right, that if Foo is virtual a null dereference of the instance pointer to do a polymorphic dispatch would cause an explosion. On the other hand, it isn't necessary to do a polymorphic dispatch (on a null pointer or otherwise) to enter the method, making it possible for the test to still be meaningful given the right circumstances (in theory). -- SunirShah
AnonymousDonor continues...
A significant amount of code that many of us execute daily does
virtual void Component::Foo() {
.... logic here
if (logic) delete this;
return;
}
Now every compiler I know of implements vtables such that this works, but I do not know for instance that delete this does not and cannot result in an unload lib that makes the next line of code invalid memory! -- AnonymousDonor
You're right - dynamic code placement is in fact a problem for delete this. Obviously, you wouldn't do this for classes that employ dynamic code placement. Fortunately, that isn't the case either. (Typically, one uses a SmartPointer wrapper to manage dynamic code, not delete this.) By the way, a related and more perplexing problem is volatile methods (i.e. volatile T *this where T is the type of the class). -- SunirShah
Optimization
I think this is merely the old debate on whether to include guards within the called function or push them back onto the calling function. In either case, I would recommend replacing the test with a more informatively named inline function, for example "bool hasBeenDestroyed(void) const". This would give the poor maintenance programmer a hint at what you are trying to accomplish as well as open doors to replace the evaluation should it lead to problems. -- AnonymousDonor
Guards inside vs. out. Maybe, but in most ways it's more efficient to put the test in one place instead of holographically throughout the code. (OnceAndOnlyOnce) The only advantage would be saving execution time, perhaps, but that's very dubious. I would think on a Pentium III if( 0 == this ) return; takes one cycle due to pipelining and branch prediction whereas if( pComponent ) pComponent->destroy(); would take a dozen cycles if 0 != pComponent. (And maybe the code cache takes a hit.) So, then you have to measure it empirically against real data, and then balance this against the complexity headache. -- SunirShah
Be careful about optimizing on the fly. First point, you optimized the exception case not the main case. Second point, subroutine calls and returns, which involve stack manipulation, tend to be some of the more expensive operations you can perform. Most guidelines for optimization that I have seen recommend pushing guard code back to the caller if this level of optimization is needed. Third point, if your system requires optimization of the microprocessor cache in order to work, you are in a highly risky situation. Either recommend faster hardware or change software. You can go from an OO approach to a functional decomposition approach (i.e. go from C++ to C) and even fall back to assembly. For best results, use your programming language in its most common form. If you cannot do this, if you cannot use constructors or destructors in C++ or need to code for caching efficiency, switch to a language that meets your needs. -- AnonymousDonor
That's exactly right; however we develop for HandHelds, which have different considerations. For instance, we can't control the hardware, and the hardware capabilities could degrade as we move to future platforms, not increase. Code size is an important optimization target as well. Optimizing a subroutine call away during runtime trades off against extra code throughout the system, which is also not good. (The rationale: how often do you destroy()?) You're right about how tricky this is; the cache argument was just a suggestion to why it's difficult to argue about optimization in a theoretical context like Wiki. Finally, removing constructors and destructors is how you go from C++ to C, which was one aspect of the exercise. Another aspect was code size reduction (25% decrease). -- SunirShah
P.S. I did write: You have to measure it empirically against real data, and then balance this against the complexity headache. I also wrote OptimizeLater. This happened to be later.
Legibility
Anonymous comment deleted/destroyed. Paraphrased: replace if( 0 == this ) with if( hasBeenDestroyed() )
hasBeenDestroyed() is incorrect. A 0 value has not been destroyed. It's just 0. The test will never be wrong; you never want to dereference or free() a 0 value.
I really don't think this is a particularly complex idea anyway. We've all delete this; once or twice. There's no real mental difference. Indeed, this (nor this) isn't mystical. Method invocation is just function invocation. Think of it like free(). Who wouldn't have liked free() to be implemented like so
void free( void *p ) {
if( p ) {
Actually free p from the active heap.
}
}
? This is indeed exactly what delete does. -- SunirShah
If hasBeenDestroyed() is not correct, it merely reinforces the point why if(pComponent) or if(0 == this) does not effectively show intent.
I doubt that. I think that if you don't understand why dereferencing null pointers is bad, or what a null pointer check looks like, you're not the target audience. If you're suggesting that most programmers don't understand pointers, well... -- SunirShah
I am saying the code gives no clue as to why the pointer might be NULL. This certainly smacks of superstitious code; I don't know why this pointer may be NULL, but bad things will happen if it is, so I'd better check for that case.
Someone is trying to destroy() a NULL pointer. Dereferencing a NULL pointer leads to crashes. -- SunirShah
Maintenance Programmers
Anonymous comment deleted/destroyed. Paraphrased: The maintenance programmer may not understand this. [ed: This is particularly salient too as I made this change a few days before my contract ended and I left on an extended vacation. -- ss]
I think the "poor maintenance programmer" is a myth. Everyone either knows or, if not, they run the unit tests. A general maintenance tip: If you want to know what a line of code does, delete it and then run the program. -- SunirShah
If the maintenance programmer is touching the code, it means there is something to be changed and the existing unit tests are not of value. There is really no need to write unnecessarily cryptic code; it takes little skill and only minor thought to right clear code.
If unit tests have no value whenever you change the code, why write unit tests? -- SunirShah
Unit tests are of value in refactoring, but if you are changing code, one of two cases exist. One, the unit test failed to catch a problem found in the code, or two, a new function is being added to the code and a new unit test is needed. The existing unit tests cannot verify the new functionality and the new unit tests are as of yet unverified.
But they very much can verify that existing functionality continues to work after the new is added. If you're adding functionality that must break an existing unit test, perhaps there is cause to pause and reconsider. -- AnonymousDonor
However, the comment that initiated this was that unit tests could be used in place of clear programming to guide a maintenance programmer in changing code. My point is that although unit tests are of value in verifying that code has not changed, they are not a substitute for clear code when code does have to be changed.
We have yet to see anyone write the expression any clearer... Here's a shot:
void destroyComponent( struct Component *pComponent ) {
if( NULL == pComponent ) return;
...
}
Here, one doesn't really even need a comment. My point is that C++ isn't much different from C+sugar. All the text on this page tries to fluff it up more than that, but the core of the language hasn't changed since 1984. -- SunirShah
Actually, assignment to this, as well as checking for this==0 at the beginning of some constructors, was legal and useful in cfront 1.2 (1987), and became obsolete only when cfront 2.0 was released (1989). -- NikitaBelenki
EditText of this page
(last edited July 10, 2010)
or FindPage with title or text search