Hi Chris,
On 01/22/2020 19:21, Chris Plummer wrote:
On 1/22/20 12:23 PM, Alex Menkov wrote:
Hi Chris,
On 01/17/2020 14:36, Chris Plummer wrote:
Hi Alex,
I assume that the following:
65 operator T* () const {
66 return m_ptr;
67 }
Is used here:
183 AutoArrayPtr<char> errmsg(new char[strlen(str) + 32]); \
184 if (errmsg == nullptr) { \
I just don't understand how this works. Somehow it seems the "T*"
operator applies to the "errmsg" reference here. This is a use of C++
operators I'm not familiar with. Can you please explain.
Yes, you are right.
errmsg is an object and nullptr is a pointer, so compiler tries to
find a way to cast types.
This cast operator is the only way to cast errmsg to pointer (char *),
so compiler uses it.
You can read more about cast operators:
https://en.cppreference.com/w/cpp/language/cast_operator
They are quite convenient.
Ok. This is probably acceptable C++, but TBH not something I really care
for. It's confusing to someone who is not fluent in this use of cast
operators. It reads funny to me because you have a local variable that
is an instance of an object (not a pointer to an object), yet are
comparing it to null.
Maybe it would be more clear to introduce bool cast operator (so to
check errmsg is invalid "if (!errmsg)" can be used), but it requires
more work to resolve "safe bool" issue:
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Safe_bool
Why is the following not placed at the end if the "if" block:
299 AddRef();
300 return S_OK;
The following was removed. It's not clear to me why it was and what
the impact is:
283 ptrIDebugOutputCallbacks->AddRef();
I updated COM object implementation (SAOutputCallbacks) to follow
"standard way".
- standard implementation for QueryInterface looks like
*ppInterface = nullptr;
if (IsEqualIID(interfaceId, __uuidof(IUnknown)) ||
IsEqualIID(interfaceId, __uuidof(MyInterface1))) {
*ppInterface = static_cast<MyInterface1*>(this);
} else if (IsEqualIID(interfaceId, __uuidof(MyInterface2))) {
*ppInterface = static_cast<MyInterface2*>(this);
} else if (IsEqualIID(interfaceId, __uuidof(MyInterface3))) {
*ppInterface = static_cast<MyInterface3*>(this);
...
} else {
return E_NOINTERFACE;
}
AddRef();
return S_OK;
}
I'm not sure what standard you are referring to. The only other uses of
E_NOINTERFACE I see are 5 uses in awt that all put the AddRef() in the
if/else blocks, not outside. Can you please explain what this standard is?
Well, openjdk implements only 3 COM interfaces.
This "standard way" is from other projects I worked on (including jdk
installer which implements a lot of COM objects).
I don't care much about this change and can revert it.
As for ref.counter - I never see ref.counter is initialized by 0 (AWT
code initialize ref.counter by 1 in ctor too).
--alex
thanks
Chris
- standard handling of ref. counter sets it to 1 during object
creation (so callers don't have to call AddRef after each creation)
So I did:
- SAOutputCallbacks() : m_refCount(0), m_msgBuffer(0) {
+ SAOutputCallbacks() : m_refCount(1), m_msgBuffer(nullptr) {
and removed unnecessary AddRef:
SAOutputCallbacks* ptrIDebugOutputCallbacks = new SAOutputCallbacks();
- ptrIDebugOutputCallbacks->AddRef();
--alex
thanks,
Chris
On 1/17/20 10:37 AM, Alex Menkov wrote:
Need 2nd reviewer.
--alex
On 01/14/2020 13:10, serguei.spit...@oracle.com wrote:
Hi Alex,
Thank you for the update!
It looks good.
Still incorrect indent:
103 ~AutoJavaString() { 104 if (m_buf) { 105
m_env->ReleaseStringUTFChars(m_str, m_buf); 106 } 107 } 108 109
operator const char* () const { 110 return m_buf; 111 } ... 133
void setReleaseMode(jint mode) {
134 releaseMode = mode;
135 }
136
137 operator jbyte* () const {
138 return bytePtr;
139 }
The comment shout start from a capital letter:
177 // verifies COM call result is S_OK, throws DebuggerException
and exits otherwise.
No need for another webrev.
Thanks,
Serguei
On 1/14/20 12:39 PM, Alex Menkov wrote:
Hi Serguei,
Thank you for the review.
updated webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_improve/webrev.02/
On 01/13/2020 16:39, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks pretty good.
Just some minor comments below.
The class AutoCOMPtr has unfixed indents.
I guess, the function AutoArrayPtr.asPtr() is not used anymore
and can be deleted.
fixed.
I'd suggest to remove first level '()' brackets in the comment to
avoid brackets nesting: 74 // manage COM 'auto' pointers (to
avoid multiple Release 75 // calls at every early (exception)
returns). => 74 // manage COM 'auto' pointers to avoid multiple
Release
75 // calls at every early (exception) returns.
done.
I'd suggest to reformat it as it was originally formatted:
297 if (IsEqualIID(interfaceId, __uuidof(IUnknown))
298 || IsEqualIID(interfaceId, __uuidof(IDebugOutputCallbacks)))
{ => 297 if (IsEqualIID(interfaceId, __uuidof(IUnknown)) ||
298 IsEqualIID(interfaceId, __uuidof(IDebugOutputCallbacks))) {
done.
The comment about S_FALSE is not that clear as S_FALSE is not
really used
(better to use consistently just one value: S_FALSE or false):
418 // S_FALSE means timeout
419
COM_VERIFY_OK_(ptrIDebugControl->WaitForEvent(DEBUG_WAIT_DEFAULT,
INFINITE),
420 "Windbg Error: WaitForEvent failed!", false);
S_FALSE is a possible result of ptrIDebugControl->WaitForEvent call.
In the case we wait infinitely, so S_FALSE is not possible and we
don't handle it separately.
I removed the comment.
NULL is used in some placesand nullptr in others.
There is some mess with 0/NULL/nullptr in the file
I fixed all NULLs and some (most likely not all) 0s.
BTW it immediately discovered misuse of NULL/0:
- if (ptrIDebugSymbols->GetModuleParameters(loaded, 0, NULL,
params.asPtr()) != S_OK) {
- THROW_NEW_DEBUGGER_EXCEPTION_("Windbg Error:
GetModuleParameters failed!", false);
- }
+ COM_VERIFY_OK_(ptrIDebugSymbols->GetModuleParameters(loaded,
nullptr, 0, params),
+ "Windbg Error: GetModuleParameters failed!",
false);
2nd arg of GetModuleParameters is a pointer and 3rd is ulong
--alex
Thanks,
Serguei
On 12/19/19 15:34, Alex Menkov wrote:
Hi all,
Please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8235846
webrev:
http://cr.openjdk.java.net/~amenkov/jdk15/WinDbg_improve/webrev.01/
Main goal of the change is to improve error reporting (we have
several bugs and need at least COM error codes for WinDbg calls).
Also the fix improves/rearranges this quite old code.
--alex
