incubator-ooo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andre Fischer ...@a-w-f.de>
Subject Re: svx/source/svdraw/svdfmtf.cxx
Date Mon, 21 May 2012 09:34:05 GMT
On 20.05.2012 13:30, Armin Le Grand wrote:
> Hi Pavel,
>
> Pavel Janík<Pavel@Janik.cz>  wrote:
>> Hi,
>>
>>>> WaE = Warning as Error.
>>
>> or Warnings Are Errors.
>>
>> Gcc option -Werror: Make all warnings into errors.
>>
>> With this option turned on, all warnings are made into errors. Our long
>> term goal is to make gcc silent.
>
> Ah, yes, I remember now. We spent some time on this years ago, but never
> coud make the whole code WaE-safe.

I thought that most of non-binfilter code was WaE-safe.

Anyway, we should still try to keep/make our code WaE-safe.  It can only 
improve its quality.

> Despite that, it's good to work on it;
> sometimes this gives good hints at weird code.
>
>> These issues are not errors per se, but e.g.:
>>
>> @@ -1330,6 +1331,7 @@
>>
>>   void ImpSdrGDIMetaFileImport::DoAction(MetaWallpaperAction&  rAct)
> Please just comment /*rAct*/
>>   {
>> +    (void) rAct;
>>       OSL_ENSURE(false, "Tried to construct SdrObject from
>> MetaWallpaperAction: not supported (!)");
>>   }
>>
>> This means that rAct is unused in the method. gcc warns about it.
>>
>> This change:
>>
>> @@ -1384,6 +1388,7 @@
>>                   case GRADIENT_ELLIPTICAL: aXGradientStyle = XGRAD_ELLIPTICAL; break;
>>                   case GRADIENT_SQUARE: aXGradientStyle = XGRAD_SQUARE; break;
>>                   case GRADIENT_RECT: aXGradientStyle = XGRAD_RECT; break;
>> +               default: break;
> Hmm. I do not have the code at hand right now, cannot tell until monday.
>>               }
>>
>>               const XFillGradientItem aXFillGradientItem(
>>
>> means that some enum value is forgotten in the switch.
>>
>> This change:
>>
>> -                            for(sal_uInt32 y(0); y<  pOld->Height(); y++)
>> +                            for(sal_Int32 y(0); y<  pOld->Height(); y++)
> Please change to sal_uInt32

Why?  Does not look like y could become negative (assuming that 
pOld->Height() is also a sal_uInt32.

>> means that we were comparing signed and unsigned value.
>>
>> I do not know if these changes are OK, thus I send the patch as I used to
>> make the module WaE free.
>
> I have not seen a patch. If you have one, please send again (maybe
> directly) and I'll happily take a look on monday. I'm currently not
> compiling on gcc, so I will not be able to guarantee, though.
>
>> Hope this helps.
>>
>> P.S. Of course warnings differ between compilers and sometims the changes look weird
etc.
>
> Yes, I remember now. Do you have a good solution for swich..case where not
> all missing entries would have to be listed?

Pavel has already done that above (add "default: break;" to your switch 
statement.


Who will check in the changes?

-Andre

>
>

Mime
View raw message