Re: frysk-imports/frysk/expunit ChangeLog Equals.j ...

Re: frysk-imports/frysk/expunit ChangeLog Equals.j ...

[Andrew Cagney]

Mark,

I don't follow these changes.  If the code stripping of the matched text 
was removed, then TestExpect and other code would start failing - was 
this tested?  And the grouping code is so that expect clients can access 
the parts that were matched, that functionality was lost?

Andrew

mark@sourceware.org wrote:
> CVSROOT:	/cvs/frysk
> Module name:	frysk-imports
> Changes by:	mark@sourceware.org	2007-03-05 13:52:02
>
> Modified files:
> 	frysk/expunit  : ChangeLog Equals.java Expect.java Match.java 
>
> Log message:
> 	* Expect.java (expectMilliseconds): Don't use unimplemented methods.
> 	* Match.java: Remove unimplemented group methods.
> 	* Equals.java: Likewise.
>
> Patches:
> http://sourceware.org/cgi-bin/cvsweb.cgi/frysk-imports/frysk/expunit/ChangeLog.diff?cvsroot=frysk&r1=1.8&r2=1.9
> http://sourceware.org/cgi-bin/cvsweb.cgi/frysk-imports/frysk/expunit/Equals.java.diff?cvsroot=frysk&r1=1.1&r2=1.2
> http://sourceware.org/cgi-bin/cvsweb.cgi/frysk-imports/frysk/expunit/Expect.java.diff?cvsroot=frysk&r1=1.8&r2=1.9
> http://sourceware.org/cgi-bin/cvsweb.cgi/frysk-imports/frysk/expunit/Match.java.diff?cvsroot=frysk&r1=1.1&r2=1.2
>
>   

Re: frysk-imports/frysk/expunit ChangeLog Equals.j ...

[Mark Wielaard]

On Tue, 2007-03-06 at 10:27 -0500, Andrew Cagney wrote:
> I don't follow these changes.  If the code stripping of the matched text 
> was removed, then TestExpect and other code would start failing - was 
> this tested?  And the grouping code is so that expect clients can access 
> the parts that were matched, that functionality was lost?

Yes this was a mistake. Fixed in the followup patch. The warnings that
the new ecj gave had me confused. Sorry about that.

Cheers,

Mark

Re: frysk-imports/frysk/expunit ChangeLog Equals.j ...

[Andrew Cagney]

Mark,

I did a quick clean revert; methods such as group(...) were still 
missing :-(.

Can you post the warnings you're seeing so you/i can figure out what 
really should be changed?  If the warnings are significantly different 
then there's the complicating problem - for the moment only you will be 
seeing and fixing those problems.  In the short term, it may be prudent 
to scale back the warnings issued by the new compiler.

Andrew

Mark Wielaard wrote:
> On Tue, 2007-03-06 at 10:27 -0500, Andrew Cagney wrote:
>   
>> I don't follow these changes.  If the code stripping of the matched text 
>> was removed, then TestExpect and other code would start failing - was 
>> this tested?  And the grouping code is so that expect clients can access 
>> the parts that were matched, that functionality was lost?
>>     
>
> Yes this was a mistake. Fixed in the followup patch. The warnings that
> the new ecj gave had me confused. Sorry about that.
>
> Cheers,
>
> Mark
>
>   

Re: frysk-imports/frysk/expunit ChangeLog Equals.j ...

[Mark Wielaard]

Hi Andrew,

On Tue, 2007-03-06 at 11:06 -0500, Andrew Cagney wrote:
> I did a quick clean revert; methods such as group(...) were still 
> missing :-(.

Yes, I didn't put those back because I thought they weren't consistently
implemented (but see below). We could do with a little bit more API
review and documentation at times to prevent misinterpretation over
required functionality.

> Can you post the warnings you're seeing so you/i can figure out what 
> really should be changed?  If the warnings are significantly different 
> then there's the complicating problem - for the moment only you will be 
> seeing and fixing those problems.  In the short term, it may be prudent 
> to scale back the warnings issued by the new compiler.

Yes, I just suggested the same in my Status mail. For now to build on
rawhide we should just use the disable-warnings patch that I attached to
this email: http://sourceware.org/ml/frysk/2007-q1/msg00173.html
And then we wait for this bug to get fixed (is already fixed upstream,
but needs a push into rawhide):
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=231020
before going over all the warning elimination again (and maybe select a
different set of warnings by default).

The new warnings that ecj gives are for unused method parameters. That
reflects the design of that API though. ecj seems to prefer abstract
methods in base classes over stubbed default implementations. I do
actually agree with ecj in this case which is why I rewrote them that
way (and left the group methods out since they weren't actually used in
the code). So ecj can be used to enforce a particular API design style,
but to do that we probably need to have a discussion about the preferred
styles first. As soon as the above bug is fixed, new packages are in
rawhide and more people have had a chance to play with the new warning
settings we should go over them and make a selection of the defaults we
actually want.

Cheers,

Mark

Re: frysk-imports/frysk/expunit ChangeLog Equals.j ...

[Andrew Cagney]

Hi Mark,

Thanks for the additional background.  That the group methods are not 
used is nothing less than a lack of test coverage - something that can 
be easily fixed.

No matter what the style, trying to locally fix warnings about unused 
inerface method parameters is a loosing game.  It is something better 
left to a code analysis tool that can see the entire code base and not 
just local interfaces and files.

Andrew


Mark Wielaard wrote:
> Hi Andrew,
>
> On Tue, 2007-03-06 at 11:06 -0500, Andrew Cagney wrote:
>   
>> I did a quick clean revert; methods such as group(...) were still 
>> missing :-(.
>>     
>
> Yes, I didn't put those back because I thought they weren't consistently
> implemented (but see below). We could do with a little bit more API
> review and documentation at times to prevent misinterpretation over
> required functionality.
>
>   
>> Can you post the warnings you're seeing so you/i can figure out what 
>> really should be changed?  If the warnings are significantly different 
>> then there's the complicating problem - for the moment only you will be 
>> seeing and fixing those problems.  In the short term, it may be prudent 
>> to scale back the warnings issued by the new compiler.
>>     
>
> Yes, I just suggested the same in my Status mail. For now to build on
> rawhide we should just use the disable-warnings patch that I attached to
> this email: http://sourceware.org/ml/frysk/2007-q1/msg00173.html
> And then we wait for this bug to get fixed (is already fixed upstream,
> but needs a push into rawhide):
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=231020
> before going over all the warning elimination again (and maybe select a
> different set of warnings by default).
>
> The new warnings that ecj gives are for unused method parameters. That
> reflects the design of that API though. ecj seems to prefer abstract
> methods in base classes over stubbed default implementations. I do
> actually agree with ecj in this case which is why I rewrote them that
> way (and left the group methods out since they weren't actually used in
> the code). So ecj can be used to enforce a particular API design style,
> but to do that we probably need to have a discussion about the preferred
> styles first. As soon as the above bug is fixed, new packages are in
> rawhide and more people have had a chance to play with the new warning
> settings we should go over them and make a selection of the defaults we
> actually want.
>
> Cheers,
>
> Mark
>
>