RFR: 8013117: Thread-local trace_buffer has wrong type and name

[Rickard Bäckman]

David,

On Apr 24, 2013, at 2:25 PM, David Holmes wrote:

> On 24/04/2013 9:05 PM, Rickard Bäckman wrote:
>> David,
>> 
>> On Apr 24, 2013, at 12:11 PM, David Holmes wrote:
>> 
>>> On 24/04/2013 7:09 PM, Rickard Bäckman wrote:
>>>> Nils,
>>>> 
>>>> no it doesn't matter. Rather intended. By initializing it to NULL we forced implementors to use a pointer that would have to be initialized at some point. Now it can be a class / struct
>>>> that is instead initialized by a default constructor.
>>> 
>>> So that addressed my question on the missing setter. But doesn't this also mean that you are now prohibiting it from being a simple pointer-type as there is no way to set it? Isn't maintaining the setter more flexible as it can be used in either case (direct assignment or copy constructor). Though lack of initialization in the current code still looks wrong.
>> 
>> Yes it makes it harder for the type to be a simple pointer, though that could be worked around by putting the pointer inside a struct. Not a great solution perhaps.
>> The other solution would be to have a setter, the question is what to initialize it with? Should we add another hook?
> 
> I don't see a truly satisfactory solution, but I think Parfait will flag that you are returning an uninitialized variable here.

Agreed.

> 
> So yes perhaps you need to define TRACE_DATA_INIT ?

Two options I think:

1) restore the setter and create a macro TRACE_DATA_DEFAULT_VALUE
2) make TRACE_DATA an empty struct.

Both works for me.

/R

> 
> David
> 
>> /R
>> 
>>> 
>>> David
>>> 
>>>> /R
>>>> 
>>>> On Apr 24, 2013, at 10:45 AM, Nils Loodin wrote:
>>>> 
>>>>> Does it matter that the pointer gets initialized to NULL before, but not now? There isn't any null checks anywhere that depends on that?
>>>>> 
>>>>> Regards,
>>>>> Nils
>>>>> 
>>>>> On 04/24/2013 09:51 AM, Rickard Bäckman wrote:
>>>>>> Hi all,
>>>>>> 
>>>>>> can I have a couple of reviews for this small change. The short story is that the current way the thread-local _trace_buffer is somewhat inflexible.
>>>>>> By changing the type of the getter this structure gets more flexible for different implementations. I also think that the name is misused. Just naming it
>>>>>> to _trace_data is more generic and less implementation-specific.
>>>>>> 
>>>>>> The webrev: http://cr.openjdk.java.net/~rbackman/8013117/
>>>>>> 
>>>>>> Thanks
>>>>>> /R
>>>>>> 
>>>>> 
>>>> 
>>