[rfc][icedtea-web] synced headers between PLUGIN_ERROR, PLUGIN_DEBUG and java

[Jiri Vanek]

On 10/31/2013 07:10 PM, Omair Majid wrote:
> Hi Jiri,
>
> * Jiri Vanek <jvanek at redhat.com> [2013-10-31 13:12]:
>> diff -r 14a8ee171687 netx/net/sourceforge/jnlp/util/logging/OutputController.java
>> --- a/netx/net/sourceforge/jnlp/util/logging/OutputController.java	Wed Oct 30 10:36:03 2013 +0100
>> +++ b/netx/net/sourceforge/jnlp/util/logging/OutputController.java	Thu Oct 31 17:57:00 2013 +0100
>> @@ -346,6 +346,7 @@
>>               if (stack != null) {
>>                   sb.append('[').append(getCallerClass(stack)).append(']');
>>               }
>> +            sb.append(" NETX Thread# ").append(Integer.toHexString(((Object)Thread.currentThread()).hashCode())).append(", name ").append(Thread.currentThread().getName());
>
> Some nits, here.
>
> You probably don't need to cast Thread.currentThread() to Object,
> hashCode is defined on all types.

I think I wont...I wont to be  clear that number I'm showing is Object's hash -"pointer". Right now 
Thread do not override it, but can...
>
> The output of this will be NETX Thread# 0x<ThreadID>, name <Thread Name>.
> Maybe it would be more readable as:
>
> NETX Thread 0x<Thread Id> [<Thread Name>]
>


If you lookto both files I patched, I'm trying to have both c++ and java headers same. In C part I'm 
mostly trying to keep what was there, as I do not fully understand pthread and gthread ..differences

Afaik gthread, nor "pthread" do not have names....


As there was both pthread and gtheread shown in debug messages, (although I'm not sure if it is 
worthy), and for java thread I'm showing also two attributes, then I wonted to have similar naming:

ITNPP Thread# %ld, gthread %p:
NETX Thread# 0x<ThreadID>, name <Thread Name>.

Although I like your suggestion for java part, it do not fit for C part.

Right now I kept original stile, feel free to rise voice again :)

> Please consider appropriate line-wrapping. Something like this is a
> little easier to read:
>
>      sb.append(" NETX Thread#")
>        .append(Integer.toHexString(((Object)Thread.currentThread()).hashCode()))
>        .append(", name ")
>        .append(Thread.currentThread.getName());

sure, fixed.

>
> I am far from an expert on C++, so take the next bit of advice with a
> grain of salt.

Still much better then me :)
>
>> +++ b/plugin/icedteanp/IcedTeaPluginUtils.h	Thu Oct 31 17:57:00 2013 +0100
>
>> +#define CREATE_HEADER(f)                                     \
>
> Please document f. Is it a FILE*, a char*, or a fd?
>
>> +  do                                                         \
>> +  {                                                          \
>> +	char s[200];                                             \
>> +    time_t t = time(NULL);                                   \
>> +    struct tm * p = localtime(&t);                           \
>
> If you are expecting to have multiple threads run this code, please use
> _r variants of function calls (localtime_r). These non-_r variants use
> static data and are not thread safe.

Can happen.. Thanx for hint.,fixed - I was not aware of _r variant :(


Thank you for review,

  J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: moreSyncedHeaders2.patch
Type: text/x-patch
Size: 5331 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131101/7bc911b7/moreSyncedHeaders2-0001.patch