C / C++ / MFC

26-Dec-07 3:09

Hello everyone,

Here is a sample from Dr. Dobb C++. In the analysis, the code is bad below.

But I do not think the code is bad,

1. if bad_alloc is thrown in new int[], we just catch it and write some log;
2. if there are any exception in B's constructor, we will also be in catch block and we could also write some log.

Why it is bad code? Any comments?

(I do not agree that there is resource leak, since if we met with bad_alloc in new int[], there is no memory allocated at all, so no root of memory/resource leak).

http://www.ddj.com/cpp/184401297

C::C(int)
try
   : B(new int[n]) // horrible!
{
   ...
}
catch(Error &e)
{
   
}

thanks in advance,
George

CPallini26-Dec-07 3:29

26-Dec-07 3:29

IMHO the problem is: if something go wrong inside B constructor the will reach the catch block, where there is no chance to release the acquired resource (i.e. allocated memory).
Smile | :)

If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler.
-- Alfonso the Wise, 13th Century King of Castile.

[my articles]

George_George26-Dec-07 3:50

26-Dec-07 3:50

Hi CPallini,

I think you mean the memory allocation of new int[] is successful, but in somewhere in constructor, there is an exception, right?

I think in this case, we can use a catch block in B's constructor to invoke delete[] to free memory, and then re-throw the exception to constructor of class C.

Any comments? Please feel free to correct me if I am wrong. Smile | :)

regards,
George

Sarath C26-Dec-07 4:09

Sarath C

26-Dec-07 4:09

My doubt is that, if B swallow the exception in its ctor, how C can know that same? B will be partially constructed without knowing that, C completed it's initialization. Those who using the class, C misunderstand that the whole construction was successful and will start using partially constructed objects. I think the error handling can't be done properly if we approach this method. Correct me if I'm wrong.

-Sarath.
"Great hopes make everything great possible" - Benjamin Franklin

My blog - Sharing My Thoughts, An Article - Understanding Statepattern

George_George26-Dec-07 18:19

26-Dec-07 18:19

Hi Sarath,

If there are any exception in B's constructor, the exception will be thrown to C's constructor. Why do you say,

Sarath. wrote:
B will be partially constructed without knowing that, C completed it's initialization.

and,

Sarath. wrote:
misunderstand that the whole construction was successful and will start using partially constructed objects.

regards,
George

CPallini26-Dec-07 4:46

26-Dec-07 4:46

Suppose you cannot change B class, B constructor hasn't a catch block able to face such a situation (after all B class doesn't even know that memory is allocated on the heap), and C class cannot access B internal data (OOP information hiding, he he Big Grin | :-D

If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler.
-- Alfonso the Wise, 13th Century King of Castile.

[my articles]

George_George26-Dec-07 18:22

26-Dec-07 18:22

Hi CPallini,

I think when there is exception in B's constructor, the exception will also be thrown to C's constructor. So, C will know its sub-components failed to be initialized in C's catch handler block. Do you agree?

(So, C will always be aware of that whether or not B's constructor fails with exceptions)

regards,
George

CPallini26-Dec-07 21:05

26-Dec-07 21:05

George_George wrote:
I think when there is exception in B's constructor, the exception will also be thrown to C's constructor.

...

George_George wrote:
(So, C will always be aware of that whether or not B's constructor fails with exceptions)

Nope:
If B handles the exception and doesn't throw again it (or a new exception) then C has no chance to handle it.

George_George wrote:
So, C will know its sub-components failed to be initialized in C's catch handler block

Even if C can handle the exception, it cannot delete the array allocated with new if C cannot access the B data members.
Smile | :)

If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler.
-- Alfonso the Wise, 13th Century King of Castile.

[my articles]

George_George26-Dec-07 21:31

26-Dec-07 21:31

Sorry, CPallini.

I may not make myself 100% understood. I mean we catch exception in B's constructor function try block handler, and free the memory in the handler, then re-throw the exception to C. Do you think it is good approach?

regards,
George

CPallini26-Dec-07 22:00

26-Dec-07 22:00

Nope.
B constructor has no knowledge about memory allocation of the passed pointer
(B can't even tell if it was allocated on the stack or on the heap) so it must not delete it. On the other hand, C, even if involved by a B re-thrown exception, cannot delete such memory having no access to B data members.
Smile | :)

If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler.
-- Alfonso the Wise, 13th Century King of Castile.

[my articles]

George_George26-Dec-07 22:36

26-Dec-07 22:36

Thanks CPallini,

Maybe I have not made my ideas clear enough. Here is the pseudo code. Please let me know what is wrong with the code. Smile | :)

class B
{
private:

int* p;

public:

    A (int* input) : p(input)
    try
    {

        // there are some other exceptions here

    }
    catch (bad_alloc)
    {
        // do not delete since bad_alloc means memory pointed by p is not allocated
        // re-throw exception to C to indicate the fail of creation of B
    }
    catch (...)
    {
        // if we do not delete p, there will be memory leak
        if (p) delete[] p;
        // re-throw exception to C to indicate the fail of creation of B
    }
}

regards,
George

CPallini26-Dec-07 22:53

26-Dec-07 22:53

George_George wrote:
Maybe I have not made my ideas clear enough.

No, IMHO I've understood your ideas Roll eyes | :rolleyes:

.

The trouble with your code come out whenever you use it the following way:

int foo[100];
B(foo);

If an exception happens inside B constructor, you'll try to delete memory allocated onto the stack!
My point is: your code is specialized to handle that particular weird situation, but it can't cope with other (more simple and reasonable) ones.
i.e. your code as flaw that my above example detects.

BTW:

George_George wrote:
A (int* input) : p(input)

The above should be

B (int* input) : p(input)

If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler.
-- Alfonso the Wise, 13th Century King of Castile.

[my articles]

George_George26-Dec-07 23:14

26-Dec-07 23:14

Thanks CPallini,

If according to component design, p is always pointed to memory on heap other than stack, do you think my code is good? Any potential issues?

regards,
George

CPallini26-Dec-07 23:34

26-Dec-07 23:34

Well, forcing the client of B to always use the heap is a very bad policy (even supposing that you are the only client of yoursef: you can forget about it Poke tongue | ;-P

, or you may simply be frustrated by, because, sometimes you may prefer to use the stack).
Anyway it is worth nothing that it doesn't help a lot because you're deleting a memory chunk allocated by the caller on unpredictable (by the caller) grounds: you delete only whenever B constructor raises an exception. This is ugly, since the caller usually (i.e. when all goes fine) relies on that chunk of memory:

int *p;
p = new int[25];
assert(p); // here George knows p is valid.
B b(p);
// here George doesn't know if p is still valid
// because B may have deleted (or not) it under the hood.

If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler.
-- Alfonso the Wise, 13th Century King of Castile.

[my articles]

George_George27-Dec-07 0:39

27-Dec-07 0:39

Thanks CPallini,

I assume that you mean if p always pointed to some object on heap, my code is bug free. Smile | :)

BTW: I agree with your analysis that always making assumption on heap is not a good idea, but in this discussion, I extract my issue in a simple way, and we should discuss under in this simple and special condition.

regards,
George

CPallini27-Dec-07 0:58

27-Dec-07 0:58

George_George wrote:
I assume that you mean if p always pointed to some object on heap, my code is bug free.

The above assumption is wrong, since, as I shown, a caller may rely on allocated memory.

int * p;
p = new int[10];
assert(p);
// fill p with some important data
...
...
B b(p);
// BUG: if B throws, here p is invalid and the important data is lost

George_George wrote:
I extract my issue in a simple way, and we should discuss under in this simple and special condition.

Under restrictive hypothesis (almost) all will work, for instance, provided B contructor never throws, the original article doesn't make sense (that doesn't mean that restrictive hypothesis are bad, they are one of the foundations of design by contract[^]).
Smile | :)

If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler.
-- Alfonso the Wise, 13th Century King of Castile.

[my articles]

George_George27-Dec-07 1:03

Re: File rename in UNICODE [modified]

27-Dec-07 1:03

Thanks CPallini,

I agree that if we do not make safe data backup, there is a bug and the memory pointed by p is deleted.

But from other point, if the data operator (class B) is destroyed, there is no use of the data from some specific perspective. Smile | :)

regards,
George

File rename in UNICODE

Ajay L D26-Dec-07 2:56

Ajay L D

26-Dec-07 2:56

Hi,
I am trying to rename a file. The new name of the file is present in another file. The problem is that the renamed file is having junk characters at the beginning. I opened the file in binary mode and found that the chars corresponded to 0xFF and 0xFE. Can anybody help me out?

Thanks!

The code I have is as follows:

{
int nRetCode = 0;

HANDLE fp = CreateFile(_T("KoreanIconNames.txt"), GENERIC_READ, FILE_SHARE_READ,
NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);

if(NULL != fp)
{
WCHAR str[BUFFER_SIZE] = {0,};
DWORD dwNumBytesRead = 0;
DWORD dwNumBytesWritten = 0;
size_t ret = 1;

ReadFile(fp, str, sizeof(str), &dwNumBytesRead, NULL);

HANDLE fpNew = CreateFile(_T("KoreanIconNamesNew.txt"), FILE_ALL_ACCESS, FILE_SHARE_READ, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

if(NULL != fpNew)
{
WriteFile(fpNew, str, sizeof(str), &dwNumBytesWritten, NULL);

CloseHandle(fpNew);
}

// Rename the file
{
CFile fileRename;
WCHAR *pNewName = str;

pNewName = wcstok(str, _T("\n"));

if(NULL != pNewName)
{
CFile::Rename(_T("Citrix.lnk"), pNewName);
}
else
{
AfxMessageBox(_T("Could not tokenize the file name string") );
}
}

CloseHandle(fp);
}
}

AJ

Sarath C26-Dec-07 4:53

Sarath C

26-Dec-07 4:53

I can't exactly understand what you are trying to do.

For copying files you can use CopyFile[^] API.

OK I'm not going into the implementation

In my knowledge it's called Byte Order Mark (BOM) which identifies the type of encoding applied to the (text) file. If you are going to rename the file, you dont have to bother about anything Windows Platform will identify it and gives correct name for that.

Please check this article.[^]

just open notepad and save text in different formats. Then open it in any hex-editor(probably visual studio can help you out) and verify each encoding.

<br />
Byte-order mark Description <br />
----------------------------<br />
EF BB BF        : UTF-8<br />
FF FE           : UTF-16, little endian<br />
FE FF           : UTF-16, big endian<br />
FF FE 00 00     : UTF-32, little endian<br />
00 00 FE FF     : UTF-32, big-endian

--Update--
Check Byte Order Mark [^] on MSDN

HTH

-Sarath.
"Great hopes make everything great possible" - Benjamin Franklin

My blog - Sharing My Thoughts, An Article - Understanding Statepattern

modified on Wednesday, December 26, 2007 11:10:08 AM

Ajay L D26-Dec-07 17:15

Ajay L D

26-Dec-07 17:15

Thanks a lot!! You are of great help!

CPallini26-Dec-07 4:56

26-Dec-07 4:56

Two suggestions:
(1) always check how many bytes you effectively read and wrote (and use that info).
(2) use the debugger.
Smile | :)

If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler.
-- Alfonso the Wise, 13th Century King of Castile.

[my articles]

Ajay L D26-Dec-07 18:46

Ajay L D

26-Dec-07 18:46

Thanks a lot!