Re: [boost] [Boost-build] Milestone 11, release candidate 3

From: Bronek Kozicki (brok_at_[hidden])
Date: 2006-07-13 16:55:16

Vladimir Prus wrote:
> While we're at it, can be have a comment for this function, at least saying what are 'parent' and
> 'child' arguments. Are they pids or something else?

these are process IDs. I will supply some comment there.

> This will be clearer if written like:
> 
>                 if (stricmp(pinfo.szExeFile, "explorer.exe") == 0)

OK.

> The last line of this comment is longer than 80 characters. At the same time, thanks for 
> detailed comments! This is tricky code so it's good to have explanations.

ok, but I think that instead of reformatting I will implement workaround 
suggested by Chris Kohlhoff. With comments, too ;)

> The comment has lines longer than 80 characters, though ;-)

OK,

>> -int related(HANDLE h, DWORD p)
>> +int related(DWORD d, DWORD p)
>>  {
>> -    return is_parent_child(get_process_id(h), p);
>> +    return is_parent_child(d, p);
>>  }

> If this function just forwards to is_parent_child, 
> can we just change all callers to call is_parent_child?

good point.

> Use sizeof(buf), perhaps? 

OK,

>>      if (strcmp(buf, "#32770"))
> 
> While we're at it, it will be clearer if written like:
> 
>        if (strcmp(buf, "#32770") != 0)

OK

> BTW, what does this code does? Do all dialogs have class name of #32770? 
> Or this is specific class of dialogs we're interested in?

all "system" dialogs have class name #32770 . It's obviously possible to 
create window that looks just like dialog but is other class, but no such 
window is known to me to display assertion or process failure report.

>> +    tid = GetWindowThreadProcessId(hwnd, &pid);
>> +    if (tid && related(p.pid, pid))
> What's the point of check for 'tid'?

GetWindowThreadProcessId returns 0 if failed (undocumented feature; I asked 
Microsoft to supplement MSDN)

>> +        PostMessageA(hwnd, WM_CLOSE, 0, 0);
>> +        /* now wait and see if it worked. If not, insist */
>> +        if (WaitForSingleObject(p.h, 200) == WAIT_TIMEOUT)
>> +        {
>> +            PostThreadMessageA(tid, WM_QUIT, 0, 0);
>> +            if (WaitForSingleObject(p.h, 500) == WAIT_TIMEOUT)
>> +            {
>> +                PostThreadMessageA(tid, WM_QUIT, 0, 0);
>> +                WaitForSingleObject(p.h, 500);
> 
> Is posting WM_QUIT message twice better than posting it once?

this is why I call it "insisting". Windows messages are sometimes a little 
unreliable (IMHO) and if first message "misses" there is small chance than 
next one will "hit" . Maybe it's needless, but it's also harmless.

> Is the point here to close dialogs right away, and not wait possibly long time for timeout? If so,
> maybe this should be added to comment?

exactly. The code to automatically close message boxes is quite useless if it 
does not close the window in reasonably short time - the whole purpose of this 
code is to let us (testers) use long process timeouts (option -l , so that 
slow compilers or tests have time to do their job) without tremendously 
slowing tests while displaying these messages. It's especially useful for all 
kinds of experiments, when these dialogs may appear quite often.

> Overall, looks reasonable. Would you like to address the comments and sent the patch again,
> or it will be more convenient if I commit the patch right now and you'll send further patches 
> when you have the time?

submit now, because the code is well tested and works, and I will try to 
prepare another patch before weekend.

B.