Re: [allsvn] r271464 - cws/perftest08/sal/util

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [allsvn] r271464 - cws/perftest08/sal/util

stephan.bergmann
> Author: mib
> Date: Mon May  4 13:21:56 2009
> New Revision: 271464
>
> Log:
> #159717#: File name infor for performance test
>
> Modified:
>    cws/perftest08/sal/util/sal.map
>
> Modified: cws/perftest08/sal/util/sal.map
> ==============================================================================
> --- cws/perftest08/sal/util/sal.map Mon May  4 13:21:46 2009 (r271463)
> +++ cws/perftest08/sal/util/sal.map Mon May  4 13:21:56 2009 (r271464)
> @@ -588,6 +588,11 @@
>          rtl_math_atanh;
>  } UDK_3.8;
>  
> +UDK_3.10 { # OOo 3.2
> +    global:
> +        rtl_logfile_hasLogFile;
> +} UDK_3.9;
> +
>  PRIVATE_1.0 {
>      global:
>          osl_detail_ObjectRegistry_storeAddresses;

> Author: mib
> Date: Mon May  4 13:21:18 2009
> New Revision: 271462
>
> Log:
> #159717#: File name infor for performance test
>
> Modified:
>    cws/perftest08/sal/inc/rtl/logfile.h
>    cws/perftest08/sal/inc/rtl/logfile.hxx
>
> Modified: cws/perftest08/sal/inc/rtl/logfile.h
> ==============================================================================
> --- cws/perftest08/sal/inc/rtl/logfile.h Mon May  4 13:13:03 2009 (r271461)
> +++ cws/perftest08/sal/inc/rtl/logfile.h Mon May  4 13:21:18 2009 (r271462)
> @@ -67,6 +67,14 @@
>  */
>  void SAL_CALL rtl_logfile_longTrace(char const * format, ...);
>  
> +/** Return if a log file is written.
> +
> + @return true if a log file is written
> +
> +    @since UDK 3.10.0

For OOo 3.2 (see above) this must be @since UDK 3.2.11, see
<http://wiki.services.openoffice.org/wiki/UNO_%40since_Tags>.

> +*/
> +int SAL_CALL rtl_logfile_hasLogFile(void);

This function should return sal_Bool instead of int.

Note that the C++ class in the same file use the spelling "Logfile"
instead of "LogFile."

> +
>  #ifdef __cplusplus
>  }
>  #endif
>
> Modified: cws/perftest08/sal/inc/rtl/logfile.hxx
> ==============================================================================
> --- cws/perftest08/sal/inc/rtl/logfile.hxx Mon May  4 13:13:03 2009 (r271461)
> +++ cws/perftest08/sal/inc/rtl/logfile.hxx Mon May  4 13:21:18 2009 (r271462)
> @@ -189,12 +189,19 @@
>  
>  #define RTL_LOGFILE_PRODUCT_TRACE( string )  \
>              rtl_logfile_longTrace( "| : %s\n", string )
> -#define RTL_LOGFILE_PRODUCT_CONTEXT( instance, name ) \
> -            ::rtl::Logfile instance( name )
>  #define RTL_LOGFILE_PRODUCT_TRACE1( frmt, arg1 ) \
>               rtl_logfile_longTrace( "| : " ); \
>               rtl_logfile_trace( frmt, arg1 ); \
>               rtl_logfile_trace( "\n" )
> +#define RTL_LOGFILE_PRODUCT_CONTEXT( instance, name ) \
> +            ::rtl::Logfile instance( name )
> +#define RTL_LOGFILE_PRODUCT_CONTEXT_TRACE1( instance, frmt, arg1 ) \
> + rtl_logfile_longTrace( "| %s : ", \
> +   instance.getName() ); \
> +             rtl_logfile_trace( frmt, arg1 ); \
> +             rtl_logfile_trace( "\n" )
> +#define RTL_LOGFILE_HASLOGFILE \
> + rtl_logfile_hasLogFile

I would argue against introducing the redundant RTL_LOGFILE_HASLOGFILE
macro.

>              
>  
>  #endif

> Author: mib
> Date: Mon May  4 13:22:27 2009
> New Revision: 271465
>
> Log:
> #159717#: File name infor for performance test
>
> Modified:
>    cws/perftest08/sal/rtl/source/logfile.cxx
>
> Modified: cws/perftest08/sal/rtl/source/logfile.cxx
> ==============================================================================
> --- cws/perftest08/sal/rtl/source/logfile.cxx Mon May  4 13:21:56 2009 (r271464)
> +++ cws/perftest08/sal/rtl/source/logfile.cxx Mon May  4 13:22:27 2009 (r271465)
> @@ -250,3 +250,7 @@
>          va_end(args);
>      }
>  }
> +
> +extern "C" int SAL_CALL rtl_logfile_hasLogFile( void ) {
> +    return g_buffer != 0;
> +}

Is it not necessary to call init() before checking g_buffer?

The explicit "void" argument list is un-idiomatic in C++.

By the way, why did you split this single change across three commits?

-Stephan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [allsvn] r271464 - cws/perftest08/sal/util

Michael Brauer - Sun Germany - ham02 - Hamburg
On 05/04/09 17:28, Stephan Bergmann wrote:

>> Author: mib
>> Date: Mon May  4 13:21:56 2009
>> New Revision: 271464
>>
>> Log:
>> #159717#: File name infor for performance test
>>
>> Modified:
>>    cws/perftest08/sal/util/sal.map
>>
>> Modified: cws/perftest08/sal/util/sal.map
>>  void SAL_CALL rtl_logfile_longTrace(char const * format, ...);
>>  
>> +/** Return if a log file is written.
>> +
>> +    @return true if a log file is written
>> +
>> +    @since UDK 3.10.0
>
> For OOo 3.2 (see above) this must be @since UDK 3.2.11, see
> <http://wiki.services.openoffice.org/wiki/UNO_%40since_Tags>.

Okay. I didn't now that. Thanks for the link.

>
>> +*/
>> +int SAL_CALL rtl_logfile_hasLogFile(void);
>
> This function should return sal_Bool instead of int.

Yes, you are right.

>
> Note that the C++ class in the same file use the spelling "Logfile"
> instead of "LogFile."

Yes, but the documentation says "class LogFile" and spells it in natural
language as "log file". This seems to be the correct spelling to me.
Translating this into Camel case result in "LogFile".

>> +#define RTL_LOGFILE_HASLOGFILE \
>> +             rtl_logfile_hasLogFile
>
> I would argue against introducing the redundant RTL_LOGFILE_HASLOGFILE
> macro.

What are your arguments against it?

My argument for introducing it is that this makes the names that are
used for the log file feature more consistent.

But I will change the macro definition to

#define RTL_LOGFILE_HASLOGFILE() \
              rtl_logfile_hasLogFile()

This will additionally allow us to replace it with

#define RTL_LOGFILE_HASLOGFILE() \
              sal_False

if we want to remove/disable the feature one day.


>> +
>> +extern "C" int SAL_CALL rtl_logfile_hasLogFile( void ) {
>> +    return g_buffer != 0;
>> +}
>
> Is it not necessary to call init() before checking g_buffer?

Yes, you are right.
>
> The explicit "void" argument list is un-idiomatic in C++.

It's C, isn't it? But if you don't like the void, I'm fine with removing it.

>
> By the way, why did you split this single change across three commits?

Well, I usually check the changes I have made directory by directory and
commit those that I have reviewed.

Thanks for the review.

Michael

>
> -Stephan


--
Michael Brauer, Technical Architect Software Engineering
StarOffice/OpenOffice.org
Sun Microsystems GmbH             Nagelsweg 55
D-20097 Hamburg, Germany          [hidden email]
http://sun.com/staroffice         +49 40 23646 500
http://blogs.sun.com/GullFOSS

Sitz der Gesellschaft: Sun Microsystems GmbH, Sonnenallee 1,
           D-85551 Kirchheim-Heimstetten
Amtsgericht Muenchen: HRB 161028
Geschaeftsfuehrer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Haering

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [allsvn] r271464 - cws/perftest08/sal/util

stephan.bergmann
On 05/05/09 09:09, Michael Brauer - Sun Germany - ham02 - Hamburg wrote:
> On 05/04/09 17:28, Stephan Bergmann wrote:
>>> +#define RTL_LOGFILE_HASLOGFILE \
>>> +             rtl_logfile_hasLogFile
>>
>> I would argue against introducing the redundant RTL_LOGFILE_HASLOGFILE
>> macro.
>
> What are your arguments against it?

Just the canonical one, that C/C++ macros are bad in general.

> My argument for introducing it is that this makes the names that are
> used for the log file feature more consistent.
>
> But I will change the macro definition to
>
> #define RTL_LOGFILE_HASLOGFILE() \
>              rtl_logfile_hasLogFile()
>
> This will additionally allow us to replace it with
>
> #define RTL_LOGFILE_HASLOGFILE() \
>              sal_False
>
> if we want to remove/disable the feature one day.

OK, I see.

>> The explicit "void" argument list is un-idiomatic in C++.
>
> It's C, isn't it? But if you don't like the void, I'm fine with removing
> it.

The file where it appears in is C++, that's why I mentioned it (but its
really just a very minor nitpick).

>> By the way, why did you split this single change across three commits?
>
> Well, I usually check the changes I have made directory by directory and
> commit those that I have reviewed.

It makes working with the code easier if you commit changes that belong
together wholesale.

-Stephan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [allsvn] r271464 - cws/perftest08/sal/util

Michael Brauer - Sun Germany - ham02 - Hamburg
On 05/05/09 09:20, Stephan Bergmann wrote:

> On 05/05/09 09:09, Michael Brauer - Sun Germany - ham02 - Hamburg wrote:
>> On 05/04/09 17:28, Stephan Bergmann wrote:
>>>> +#define RTL_LOGFILE_HASLOGFILE \
>>>> +             rtl_logfile_hasLogFile
>>>
>>> I would argue against introducing the redundant
>>> RTL_LOGFILE_HASLOGFILE macro.
>>
>> What are your arguments against it?
>
> Just the canonical one, that C/C++ macros are bad in general.

Agreed. But, well, I'm extending a feature that is already based on
macros, and it is not my intention to change that.

>>> The explicit "void" argument list is un-idiomatic in C++.
>>
>> It's C, isn't it? But if you don't like the void, I'm fine with
>> removing it.
>
> The file where it appears in is C++, that's why I mentioned it (but its
> really just a very minor nitpick).

But the file has the extension "c" although we usually use "cxx" for C++
files. So, it seems that its naming is a little bit inconsistent. The
function itself has an 'extern "C"'.

Michael

--
Michael Brauer, Technical Architect Software Engineering
StarOffice/OpenOffice.org
Sun Microsystems GmbH             Nagelsweg 55
D-20097 Hamburg, Germany          [hidden email]
http://sun.com/staroffice         +49 40 23646 500
http://blogs.sun.com/GullFOSS

Sitz der Gesellschaft: Sun Microsystems GmbH, Sonnenallee 1,
           D-85551 Kirchheim-Heimstetten
Amtsgericht Muenchen: HRB 161028
Geschaeftsfuehrer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Haering

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: [allsvn] r271464 - cws/perftest08/sal/util

stephan.bergmann
On 05/05/09 09:29, Michael Brauer - Sun Germany - ham02 - Hamburg wrote:

> On 05/05/09 09:20, Stephan Bergmann wrote:
>> On 05/05/09 09:09, Michael Brauer - Sun Germany - ham02 - Hamburg wrote:
>>> On 05/04/09 17:28, Stephan Bergmann wrote:
>>>>> +#define RTL_LOGFILE_HASLOGFILE \
>>>>> +             rtl_logfile_hasLogFile
>>>>
>>>> I would argue against introducing the redundant
>>>> RTL_LOGFILE_HASLOGFILE macro.
>>>
>>> What are your arguments against it?
>>
>> Just the canonical one, that C/C++ macros are bad in general.
>
> Agreed. But, well, I'm extending a feature that is already based on
> macros, and it is not my intention to change that.

No, of course you do not want to change the existing stuff.  But if some
new part of the feature can be a plain C function, why bother wrapping
it up in a C macro?  (Anyway, you gave already gave a rationale for
doing so, so this discussion starts to become academic.)

>>>> The explicit "void" argument list is un-idiomatic in C++.
>>>
>>> It's C, isn't it? But if you don't like the void, I'm fine with
>>> removing it.
>>
>> The file where it appears in is C++, that's why I mentioned it (but
>> its really just a very minor nitpick).
>
> But the file has the extension "c" although we usually use "cxx" for C++
> files. So, it seems that its naming is a little bit inconsistent. The
> function itself has an 'extern "C"'.

I was talking about cws/perftest08/sal/rtl/source/logfile.cxx.

-Stephan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: [allsvn] r271464 - cws/perftest08/sal/util

Michael Brauer - Sun Germany - ham02 - Hamburg
On 05/05/09 09:45, Stephan Bergmann wrote:
>> But the file has the extension "c" although we usually use "cxx" for
>> C++ files. So, it seems that its naming is a little bit inconsistent.
>> The function itself has an 'extern "C"'.
>
> I was talking about cws/perftest08/sal/rtl/source/logfile.cxx.

Yes, sure. Seems that I got confused because the file that contains the
the function prototypes of the new but also the existing rtl_logfile
methods is named logfile.h. But I think this discussion is also getting
a little bit academic.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]