Re: [Libreoffice] Mi first very little patch

2011-09-20 Thread CaStarCo 
2011/9/20 Lionel Elie Mamane 

> On Mon, Sep 12, 2011 at 10:45:31PM +0200, CaStarCo wrote:
>
> > @Lionel , What is the QuadPart member of the structure? The high
> > part? the low part? or another thing?
>
> The union is fully described at
> http://msdn.microsoft.com/en-us/library/aa383713(v=vs.85).aspx
>
> Are you familiar with the concept of C/C++ union? If not, Google gives
> a few hits that don't seem too bad at first sight:
>
>
Ok, thanks! :) I know the concept of union, my problem was the definition of
"QuadPart".


>  http://www.cplusplus.com/doc/tutorial/other_data_types/#union
>  http://www.go4expert.com/forums/showthread.php?t=15
>  http://en.wikipedia.org/wiki/Union_(computer_science)
>  http://msdn.microsoft.com/en-us/library/y9zewe0d(v=vs.80).aspx
>
> To get to your question, the QuadPart is the whole 64 bits, that is
> the combination of the high part *and* the low part.
>
> Essentially, the paragraph I quoted says that to treat the FILETIME
> value as a 64bit integer, one should proceed as such:
>
> __int64 Value;
> FILETIME *ft = // some value
> ULARGE_INTEGER t;
> t.LowPart = ft->dwLowDateTime;
> t.HighPart = ft->dwHighDateTime;
> // Now, t.QuadPart contains the right 64 bits integer
> Value = t.QuadPart; // or use t.QuadPart directly instead of Value
>
> instead of:
>
> Value = *(__int64*)ft;
>
>
>
It's true, using unions is clearer and cleaner than using forced type
castings.


> Do not hesitate to CC to me any additional question you have on this
> matter.
>
>
> > 2011/9/12 Lionel Elie Mamane 
>
> >> On Sun, Sep 11, 2011 at 10:04:52PM +0200, CaStarCo wrote:
> >>> I've created a second (very little too) patch to reduce the scope of a
> >>> variable.
>
> >>> --- a/sal/osl/w32/file_dirvol.cxx
> >>> +++ b/sal/osl/w32/file_dirvol.cxx
> >>> @@ -60,7 +60,6 @@ extern "C" BOOL TimeValueToFileTime(const TimeValue
> *cpTimeVal, FILETIME *pFTime
>
> >> Thank you for your contribution to LibreOffice, it is most
> >> welcome. Looking at that function in that file, it needs a more
> >> thorough cleanup, and maybe other functions in that file and/or in
> >> other files in the same directory; see
> >> http://msdn.microsoft.com/en-us/library/ms724284%28v=VS.85%29.aspx, in
> >> particular:
>
> >>  It is not recommended that you add and subtract values from the
> >>  FILETIME structure to obtain relative times. Instead, you should copy
> >>  the low- and high-order parts of the file time to a ULARGE_INTEGER
> >>  structure, perform 64-bit arithmetic on the QuadPart member, and copy
> >>  the LowPart and HighPart members into the FILETIME structure.
>
> >>  Do not cast a pointer to a FILETIME structure to either a
> >>  ULARGE_INTEGER* or __int64* value because it can cause alignment
> >>  faults on 64-bit Windows.
>
> >> It would be best to look at every place that FILETIME is used in that
> >> directory.
>
> >> Just in case you want to volunteer for that cleanup :)
>
> >> When you send a patch, could you please confirm the patch is licensed
> >> under MPL1.1/LGPLv3+? To make things easier, you can do a one-time
> >> blanket "all my patches" thing if you wish, sort of common to do that.
>
> --
> Lionel
>
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] Mi first very little patch

2011-09-20 Thread Lionel Elie Mamane 
On Mon, Sep 12, 2011 at 10:45:31PM +0200, CaStarCo wrote:

> @Lionel , What is the QuadPart member of the structure? The high
> part? the low part? or another thing?

The union is fully described at
http://msdn.microsoft.com/en-us/library/aa383713(v=vs.85).aspx

Are you familiar with the concept of C/C++ union? If not, Google gives
a few hits that don't seem too bad at first sight:

 http://www.cplusplus.com/doc/tutorial/other_data_types/#union
 http://www.go4expert.com/forums/showthread.php?t=15
 http://en.wikipedia.org/wiki/Union_(computer_science)
 http://msdn.microsoft.com/en-us/library/y9zewe0d(v=vs.80).aspx

To get to your question, the QuadPart is the whole 64 bits, that is
the combination of the high part *and* the low part.

Essentially, the paragraph I quoted says that to treat the FILETIME
value as a 64bit integer, one should proceed as such:

__int64 Value;
FILETIME *ft = // some value
ULARGE_INTEGER t;
t.LowPart = ft->dwLowDateTime;
t.HighPart = ft->dwHighDateTime;
// Now, t.QuadPart contains the right 64 bits integer
Value = t.QuadPart; // or use t.QuadPart directly instead of Value

instead of:

Value = *(__int64*)ft;


Do not hesitate to CC to me any additional question you have on this
matter.


> 2011/9/12 Lionel Elie Mamane 

>> On Sun, Sep 11, 2011 at 10:04:52PM +0200, CaStarCo wrote:
>>> I've created a second (very little too) patch to reduce the scope of a
>>> variable.

>>> --- a/sal/osl/w32/file_dirvol.cxx
>>> +++ b/sal/osl/w32/file_dirvol.cxx
>>> @@ -60,7 +60,6 @@ extern "C" BOOL TimeValueToFileTime(const TimeValue 
>>> *cpTimeVal, FILETIME *pFTime

>> Thank you for your contribution to LibreOffice, it is most
>> welcome. Looking at that function in that file, it needs a more
>> thorough cleanup, and maybe other functions in that file and/or in
>> other files in the same directory; see
>> http://msdn.microsoft.com/en-us/library/ms724284%28v=VS.85%29.aspx, in
>> particular:

>>  It is not recommended that you add and subtract values from the
>>  FILETIME structure to obtain relative times. Instead, you should copy
>>  the low- and high-order parts of the file time to a ULARGE_INTEGER
>>  structure, perform 64-bit arithmetic on the QuadPart member, and copy
>>  the LowPart and HighPart members into the FILETIME structure.

>>  Do not cast a pointer to a FILETIME structure to either a
>>  ULARGE_INTEGER* or __int64* value because it can cause alignment
>>  faults on 64-bit Windows.

>> It would be best to look at every place that FILETIME is used in that
>> directory.

>> Just in case you want to volunteer for that cleanup :)

>> When you send a patch, could you please confirm the patch is licensed
>> under MPL1.1/LGPLv3+? To make things easier, you can do a one-time
>> blanket "all my patches" thing if you wish, sort of common to do that.

-- 
Lionel
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] Mi first very little patch

2011-09-12 Thread CaStarCo 
@Lionel , What is the QuadPart member of the structure? The high part? the
low part? or another thing?

Thanks in advance :) .

2011/9/12 Lionel Elie Mamane 

> On Sun, Sep 11, 2011 at 10:04:52PM +0200, CaStarCo wrote:
> > I've created a second (very little too) patch to reduce the scope of a
> > variable.
>
> > --- a/sal/osl/w32/file_dirvol.cxx
> > +++ b/sal/osl/w32/file_dirvol.cxx
> > @@ -60,7 +60,6 @@ extern "C" BOOL TimeValueToFileTime(const TimeValue
> *cpTimeVal, FILETIME *pFTime
>
> Thank you for your contribution to LibreOffice, it is most
> welcome. Looking at that function in that file, it needs a more
> thorough cleanup, and maybe other functions in that file and/or in
> other files in the same directory; see
> http://msdn.microsoft.com/en-us/library/ms724284%28v=VS.85%29.aspx, in
> particular:
>
>  It is not recommended that you add and subtract values from the
>  FILETIME structure to obtain relative times. Instead, you should copy
>  the low- and high-order parts of the file time to a ULARGE_INTEGER
>  structure, perform 64-bit arithmetic on the QuadPart member, and copy
>  the LowPart and HighPart members into the FILETIME structure.
>
>  Do not cast a pointer to a FILETIME structure to either a
>  ULARGE_INTEGER* or __int64* value because it can cause alignment
>  faults on 64-bit Windows.
>
> It would be best to look at every place that FILETIME is used in that
> directory.
>
> Just in case you want to volunteer for that cleanup :)
>
> When you send a patch, could you please confirm the patch is licensed
> under MPL1.1/LGPLv3+? To make things easier, you can do a one-time
> blanket "all my patches" thing if you wish, sort of common to do that.
>
>
> Best Regards,
>
> --
> Lionel
>



-- 
- Per la llibertat del coneixement -
- Per la llibertat de la ment...   -
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] Mi first very little patch

2011-09-12 Thread CaStarCo 
Thanks you very much :) ,

I'll take a look and try to make better patches. And, of course, all my
patches will be licensed under MPL1.1/LGPLv3+ . ^_^ .
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] Mi first very little patch

2011-09-12 Thread Stephan Bergmann 

On 09/12/2011 01:03 AM, Lionel Elie Mamane wrote:

On Sun, Sep 11, 2011 at 10:04:52PM +0200, CaStarCo wrote:

I've created a second (very little too) patch to reduce the scope of a
variable.



--- a/sal/osl/w32/file_dirvol.cxx
+++ b/sal/osl/w32/file_dirvol.cxx
@@ -60,7 +60,6 @@ extern "C" BOOL TimeValueToFileTime(const TimeValue 
*cpTimeVal, FILETIME *pFTime


Thank you for your contribution to LibreOffice, it is most
welcome. Looking at that function in that file, it needs a more
thorough cleanup, and maybe other functions in that file and/or in
other files in the same directory; see
http://msdn.microsoft.com/en-us/library/ms724284%28v=VS.85%29.aspx, in
particular:

[...]

Also, the scope of localTime can be reduced even further, changing the 
line that assigns it into also declaring it, as in


  __int64 localTime = cpTimeVal->...;

-Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] Mi first very little patch

2011-09-11 Thread Lionel Elie Mamane 
On Sun, Sep 11, 2011 at 10:04:52PM +0200, CaStarCo wrote:
> I've created a second (very little too) patch to reduce the scope of a
> variable.

> --- a/sal/osl/w32/file_dirvol.cxx
> +++ b/sal/osl/w32/file_dirvol.cxx
> @@ -60,7 +60,6 @@ extern "C" BOOL TimeValueToFileTime(const TimeValue 
> *cpTimeVal, FILETIME *pFTime

Thank you for your contribution to LibreOffice, it is most
welcome. Looking at that function in that file, it needs a more
thorough cleanup, and maybe other functions in that file and/or in
other files in the same directory; see
http://msdn.microsoft.com/en-us/library/ms724284%28v=VS.85%29.aspx, in
particular:

 It is not recommended that you add and subtract values from the
 FILETIME structure to obtain relative times. Instead, you should copy
 the low- and high-order parts of the file time to a ULARGE_INTEGER
 structure, perform 64-bit arithmetic on the QuadPart member, and copy
 the LowPart and HighPart members into the FILETIME structure.

 Do not cast a pointer to a FILETIME structure to either a
 ULARGE_INTEGER* or __int64* value because it can cause alignment
 faults on 64-bit Windows.

It would be best to look at every place that FILETIME is used in that
directory.

Just in case you want to volunteer for that cleanup :)

When you send a patch, could you please confirm the patch is licensed
under MPL1.1/LGPLv3+? To make things easier, you can do a one-time
blanket "all my patches" thing if you wish, sort of common to do that.


Best Regards,

-- 
Lionel
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] Mi first very little patch

2011-09-11 Thread Norbert Thiebaud 
On Sun, Sep 11, 2011 at 2:39 PM, CaStarCo  wrote:
> Hello, I've created a very little patch to solve the next CppCheck warning (
> http://libreoffice.boldandbusted.com/464.html ) . (This is my first patch in
> LibreOffice)

Welcome to libreoffice and thanks for the patch.

Unfortunately you got the parenthses wrong...

--- a/sw/source/core/crsr/findfmt.cxx
+++ b/sw/source/core/crsr/findfmt.cxx
@@ -61,7 +61,7 @@ sal_Bool SwPaM::Find( const SwFmt& rFmt, SwMoveFn fnMove,
 while( !bFound &&
 0 != ( pNode = ::GetNode( *pPam, bFirst, fnMove, bInReadOnly )))
 {
-if( 0 != ( bFound = pNode->GetFmtColl() == &rFmt ))
+if( 0 != ( ( bFound = pNode->GetFmtColl() ) == &rFmt ))

here bFound is a boolean. The intent is to check if pNode->getFmtCol()
is equal to &rFmt. the original author decided to store that test in
boFound (although that is not needed here, because if boFound is true
(ie != 0) then we break out of the while loop and never need to test
boFound again...


so

the minimum patch should have been

+if( 0 != ( bFound = (pNode->GetFmtColl()  == &rFmt )


a more 'involved' clean-up could have been (considering that th
function already has more than one return point)

diff --git a/sw/source/core/crsr/findfmt.cxx b/sw/source/core/crsr/findfmt.cxx
index 7a548c3..5b29686 100644
--- a/sw/source/core/crsr/findfmt.cxx
+++ b/sw/source/core/crsr/findfmt.cxx
@@ -37,7 +37,6 @@
 sal_Bool SwPaM::Find( const SwFmt& rFmt, SwMoveFn fnMove,
 const SwPaM *pRegion, sal_Bool bInReadOnly  )
 {
-sal_Bool bFound = sal_False;
 sal_Bool bSrchForward = fnMove == fnMoveForward;
 SwPaM* pPam = MakeRegion( fnMove, pRegion );

@@ -58,10 +57,9 @@ sal_Bool SwPaM::Find( const SwFmt& rFmt, SwMoveFn fnMove,

 sal_Bool bFirst = sal_True;
 SwCntntNode* pNode;
-while( !bFound &&
-0 != ( pNode = ::GetNode( *pPam, bFirst, fnMove, bInReadOnly )))
+while( (pNode = ::GetNode( *pPam, bFirst, fnMove, bInReadOnly )) != 0)
 {
-if( 0 != ( bFound = pNode->GetFmtColl() == &rFmt ))
+if( pNode->GetFmtColl() == &rFmt )
 {
 // wurde die FormatCollection gefunden, dann handelt es sich auf
 // jedenfall um einen SwCntntNode !!
@@ -75,11 +73,13 @@ sal_Bool SwPaM::Find( const SwFmt& rFmt, SwMoveFn fnMove,
 GetMark()->nContent = 0;
 if( !bSrchForward ) // rueckwaerts Suche?
 Exchange(); // SPoint und GetMark tauschen
+delete pPam;
+return sal_True;
 break;
 }
 }
 delete pPam;
-return bFound;
+return sal_False;
 }

 Norbert
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] Mi first very little patch

2011-09-11 Thread CaStarCo 
Ups :( , my patch introduced a bug, it's not valid.

2011/9/11 CaStarCo 

> Hello, I've created a very little patch to solve the next CppCheck warning
> ( http://libreoffice.boldandbusted.com/464.html ) . (This is my first
> patch in LibreOffice)
>
>


-- 
- Per la llibertat del coneixement -
- Per la llibertat de la ment...   -
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: [Libreoffice] Mi first very little patch

2011-09-11 Thread CaStarCo 
I've created a second (very little too) patch to reduce the scope of a
variable.
From 7c434fd7af9892fee7b120e00a135d01693265af Mon Sep 17 00:00:00 2001
From: Andreu Correa Casablanca 
Date: Sun, 11 Sep 2011 22:01:10 +0200
Subject: [PATCH] Reduced the scope of a variable in core/sal/osl/w32/file_dirvol.cxx

---
 sal/osl/w32/file_dirvol.cxx |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sal/osl/w32/file_dirvol.cxx b/sal/osl/w32/file_dirvol.cxx
index b6dc002..e5852eb 100644
--- a/sal/osl/w32/file_dirvol.cxx
+++ b/sal/osl/w32/file_dirvol.cxx
@@ -60,7 +60,6 @@ extern "C" BOOL TimeValueToFileTime(const TimeValue *cpTimeVal, FILETIME *pFTime
 SYSTEMTIME  BaseSysTime;
 FILETIMEBaseFileTime;
 FILETIMEFTime;
-__int64 localTime;
 BOOLfSuccess = FALSE;
 
 BaseSysTime.wYear = 1970;
@@ -77,7 +76,9 @@ extern "C" BOOL TimeValueToFileTime(const TimeValue *cpTimeVal, FILETIME *pFTime
 
 if ( SystemTimeToFileTime(&BaseSysTime, &BaseFileTime) )
 {
+__int64 localTime;
 __int64 timeValue;
+
 localTime=cpTimeVal->Seconds*(__int64)1000+cpTimeVal->Nanosec/100;
 *(__int64 *)&FTime=localTime;
 fSuccess = 0 <= (timeValue= *((__int64 *)&BaseFileTime) + *((__int64 *) &FTime));
-- 
1.7.4.1

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice

[Libreoffice] Mi first very little patch

2011-09-11 Thread CaStarCo 
Hello, I've created a very little patch to solve the next CppCheck warning (
http://libreoffice.boldandbusted.com/464.html ) . (This is my first patch in
LibreOffice)
From b7646c6393ae557c9f3e11ea68651fca0589360c Mon Sep 17 00:00:00 2001
From: Andreu Correa Casablanca 
Date: Sun, 11 Sep 2011 21:28:37 +0200
Subject: [PATCH] Clarified the CppCheck report: http://libreoffice.boldandbusted.com/464.html

---
 sw/source/core/crsr/findfmt.cxx |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sw/source/core/crsr/findfmt.cxx b/sw/source/core/crsr/findfmt.cxx
index 7a548c3..0567c6e 100644
--- a/sw/source/core/crsr/findfmt.cxx
+++ b/sw/source/core/crsr/findfmt.cxx
@@ -61,7 +61,7 @@ sal_Bool SwPaM::Find( const SwFmt& rFmt, SwMoveFn fnMove,
 while( !bFound &&
 0 != ( pNode = ::GetNode( *pPam, bFirst, fnMove, bInReadOnly )))
 {
-if( 0 != ( bFound = pNode->GetFmtColl() == &rFmt ))
+if( 0 != ( ( bFound = pNode->GetFmtColl() ) == &rFmt ))
 {
 // wurde die FormatCollection gefunden, dann handelt es sich auf
 // jedenfall um einen SwCntntNode !!
-- 
1.7.4.1

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice