Pointing to the void
-
The function prototype from the interface header for a library:MY_EXPORT(void*) StartSession();The function implementation:MY_EXPORT(void*) StartSession(){ErrorFunc(0, SET);void *temp = (void *)new MyClass;if (ErrorFunc(0, GET) < eNo_Err){ delete temp;return NULL;}return temp;}How many WTFs can you find?
-
I'm assuming this is C... Um, it's difficult to tell what all the WTFs are given the information we know. There's the obvious one that it's returning a pointer to a local variable, which is never a good thing. As for the rest of it, well it looks like the function is kinda trying to be a Singleton implementation of sorts, but there's no way of telling exactly what it does unless we know what MY_EXPORT, SET and GET are (presumably macros of some sort), and what ErrorFunc is supposed to do.
Heh, and it leaks temp when it returns without an error - no guarantee that temp is still usable (very probably not) but it'll just sit there eating up memory anyway. That's quite cute.
-
No, it's C++, straight C doesn't have the 'new' keyword or classes. Let's see...
- ErrorFunc's operation depends on one of its paramers and does one of two completely different things.
- Casting a non-const pointer to void* -- this is an implicit conversion. (There is an equivalent conversion from const T* to const void*.)
- Returning a pointer to an instance of MyClass with the static type void*.
- (Win32) Returning memory allocated by new across a module boundary, if MY_EXPORT is a macro for declspec(dllexport/import).
- Returning NULL on failure rather than throwing an exception.
- Really alarming code style. Statements starting on the same line after a { or } character, specifically.
- deleteing a void* rather than a pointer of the correct type; if this works at all it's a miracle as I believe this is undefined behaviour.
- Magic parameter '0' to ErrorFunc
- "ErrorFunc"?
-
1) If can't be C if it uses "new". My guess is C++.
2) It does not return a pointer to a local variable. It returns the value of a local variable which is the pointer to an object on the heap.
3) This function does not leak temp - it uses a perfectly acceptable method for passing ownership of a memory block to the caller.
-
[quote user="GettinSadda"]3) This function does not leak temp - it uses a perfectly acceptable method for passing ownership of a memory block to the caller.[/quote] It does, however, leak anything that temp may own if the error detection fires.
-
The function doesn't do anything other than instantiating an object and returning a pointer to it.
Why not just create the object directly?
-
[quote user="Bellinghman"][quote user="GettinSadda"]3) This function does not leak temp - it uses a perfectly acceptable method for passing ownership of a memory block to the caller.[/quote] It does, however, leak anything that temp may own if the error detection fires.[/quote]
Maybe you missed the "delete temp;" in the if block that is executed on an error.
-
[quote user="GettinSadda"]
[quote user="Bellinghman"][quote user="GettinSadda"]3) This function does not leak temp - it uses a perfectly acceptable method for passing ownership of a memory block to the caller.[/quote] It does, however, leak anything that temp may own if the error detection fires.[/quote]
Maybe you missed the "delete temp;" in the if block that is executed on an error.
[/quote]
it has been my experience that any and all times you create a new instance of anything, that you must delete that instance before the program terminates. clearly this happens upon error, but what if no error occurs? temp is still sitting there sucking away on that juicy filet mignon that we like to call RAM. :) Regardless, it is always safe to delete all new instances even if upon a return of said instance due to a possible memory leak, doesn't mean the leak will happen, just means that it could, and therefore cause some major issues later when it comes to available memory to run the program again. and really, it is never a good idea to return a new instance, instead there should be a separate void pointer that receives the same information from the new void pointer and the new pointer deleted before the return is called.
-
The real WTF is your reply. Why would this be a memory leak? It's up to whatever called this function to de-allocate the result.
And why wouldn't it be a good idea to return a new instance? Why would you want to copy a void pointer to another void pointer before returning it? WTF??
-
1. Rationale: the programmer might want to return a void * rather than MyClass * because the function is part of a pimpl class; he wants to hide implementation details from the caller.
2. Defensive programming: the way the programmer tests the return of ErrorFunc() is fragile and requires knowledge of details of error handling routines; it would be better to abstract the test : if IsError( ErrorFunc(0, GET)) { ... } An exception may or may not be more appropriate, depending on the context of the calling program.
3. Bug: the compiler will try to call the destructor to type void. The programmer needs to cast temp: delete (MyClass *)temp;
4. Style: it would be better to declare temp within the function as type MyClass *, then cast the return value to void *.
5. Ok: It happens from time to time that a function expects the caller to free resources allocated by the function. The caller would be wise to use an auto ptr if he can.
Neil
-
To be honest, this looks like a C++ function that is intended to be exported for use in a C module. Since C doesn't understand classes, it has to be returned as a void *. I'm guessing the C code uses it as a sort of handle, passing it to the C++ module to do any real work on it.
The only WTF (and its not a terrible one) is that someone wrote a library in C++ that is intented to be used in C.
-
That's not a wtf at all. People write libraries in all sorts of languages and interface them with C. C is often used as the intermeditary between two languages simply because it's so close to the hardware and it's calling conventions are very well defined.
-
"< eNo_Err" relies on the specific magic values chosen. "!= eNo_Err" would not have that problem, provided that ErrorFunc(0, GET) returns eNo_Err whenever there is no error, and something else whenever there is. (On the gripping hand, someone has already mentioned throwing exceptions as a superior alternative.)
-
[quote user="emurphy"]
"< eNo_Err" relies on the specific magic values chosen. "!= eNo_Err" would not have that problem, provided that ErrorFunc(0, GET) returns eNo_Err whenever there is no error, and something else whenever there is. (On the gripping hand, someone has already mentioned throwing exceptions as a superior alternative.)
[/quote]
It all really depends on the spec for the system. For all you know, it may say that ErrorFunc() returns a code that is eNo_Err or greater on success and any lower number on error.
Also there are any number of valid reasons to not throw exception - a very, very good one has already been given; it looks like this code may be a C++ library that is called from C code. You can't go throwing exceptions to a C caller!
-
[quote user="Angstrom"]
No, it's C++, straight C doesn't have the 'new' keyword or classes. Let's see...
- ErrorFunc's operation depends on one of its paramers and does one of two completely different things.
- Casting a non-const pointer to void* -- this is an implicit conversion. (There is an equivalent conversion from const T* to const void*.)
- Returning a pointer to an instance of MyClass with the static type void*.
- (Win32) Returning memory allocated by new across a module boundary, if MY_EXPORT is a macro for declspec(dllexport/import).
- Returning NULL on failure rather than throwing an exception.
- Really alarming code style. Statements starting on the same line after a { or } character, specifically.
- deleteing a void* rather than a pointer of the correct type; if this works at all it's a miracle as I believe this is undefined behaviour.
- Magic parameter '0' to ErrorFunc
- "ErrorFunc"?
The cast to void * is because this library was written in C++, but the interface is in C. From the user's point of view, MyClass is an opaque type. It's the same reason the function returns NULL rather than throwing an exception. You're right about the other WTFs, and I didn't know about the limit on passing "new"-allocated memory across module boundaries -- the library has the option to be compiled as a DLL.
-
The cast to void* is strictly unnecessary and demonstrates a weak understanding of the language. Standard C++ allows any non-const pointer type to be directly assigned to void* or const void* and any const pointer to be directly assigned to const void* with no cast. Consider the following:
[owen@verdandi Code]$ cat castvoid.cpp
#include <iostream>
using std::cout;
using std::endl;
int main () {
int *p = new int (5);
void *v = p;
cout << p << " == " << v << endl;
delete p;
}
[owen@verdandi Code]$ CXXFLAGS="-std=c++98 -W -Wall -pedantic" make castvoid
g++ -std=c++98 -W -Wall -pedantic castvoid.cpp -o castvoid
[owen@verdandi Code]$ ./castvoid
0x93fc008 == 0x93fc008
