atom feed29 messages in net.java.dev.phoneme.advancedRe: Code review for new code submissi...
FromSent OnAttachments
Hinkmond WongApr 24, 2008 6:05 pm 
Danila SinopalnikovApr 25, 2008 1:41 am 
Davy PreuveneersApr 25, 2008 2:45 am 
Danila SinopalnikovApr 25, 2008 3:08 am 
Jiangli ZhouApr 25, 2008 9:55 am 
Davy PreuveneersMay 5, 2008 2:48 am.diff
Stephen FloresMay 8, 2008 7:41 pm 
Jiangli ZhouMay 12, 2008 11:44 am 
Davy PreuveneersMay 13, 2008 1:01 am 
Jiangli ZhouMay 13, 2008 9:25 am 
Davy PreuveneersMay 13, 2008 12:06 pm.diff
Stephen FloresMay 13, 2008 6:48 pm 
phon...@mobileandembedded.orgMay 23, 2008 6:16 pm 
Davy PreuveneersMay 24, 2008 1:14 am.diff, .diff, .diff, 3 more
phon...@mobileandembedded.orgMay 27, 2008 5:25 pm 
phon...@mobileandembedded.orgJun 4, 2008 6:26 pm 
phon...@mobileandembedded.orgJun 14, 2008 1:13 pm 
phon...@mobileandembedded.orgJun 14, 2008 9:18 pm 
xyzzy (Dean)Jun 15, 2008 1:42 am 
phon...@mobileandembedded.orgJun 15, 2008 3:16 am 
xyzzy (Dean)Jun 16, 2008 12:47 pm 
phon...@mobileandembedded.orgJun 16, 2008 1:56 pm 
phon...@mobileandembedded.orgJun 16, 2008 3:36 pm 
phon...@mobileandembedded.orgJun 16, 2008 3:55 pm 
phon...@mobileandembedded.orgJun 16, 2008 3:56 pm 
Hinkmond WongJun 16, 2008 3:57 pm 
phon...@mobileandembedded.orgJun 16, 2008 4:24 pm 
phon...@mobileandembedded.orgJun 16, 2008 4:33 pm 
phon...@mobileandembedded.orgAug 22, 2008 9:50 pm 
Subject:Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
From:Davy Preuveneers (davy@cs.kuleuven.be)
Date:Apr 25, 2008 2:45:14 am
List:net.java.dev.phoneme.advanced

Hi Danila,

Indeed you are quite right, I made this modification a while ago to avoid the warnings when compiling with eVC4 to target WM2003. I did not notice the top visCPP test and assumed my patch worked because I had USE_VS2005 set to false but still needed the -D_CRT_SECURE_NO_DEPRECATE flag.

I think I was also a bit confused, because the documentation for the compiler section says "Visual Studio 6++". I found it rather weird to have a test "ifeq ($(USE_VS2005), true)" test in that section. Even below, you have another compiler section "Embedded Visual C++ 3.0" with another USE_VS2005 test.

Is there a reason why the -D_CRT_SECURE_NO_DEPRECATE flag can only be set when USE_VS2005 is true in the visCPP section? If there is such a case, would it make sense to also add and use another variable (say USE_EVC4) instead? If not, perhaps the "ifeq ($(USE_VS2005), true)" test can be removed?

Best regards, Davy

On Friday 25 April 2008 10:41:46 Danila Sinopalnikov wrote:

Hi Davy,

regarding the second chunk of CLDC patch:

@@ -851,7 +855,10 @@ CPP_DEF_FLAGS_release = CPP_DEF_FLAGS_product = -DPRODUCT

-ifeq ($(USE_VS2005), true) +ifeq ($(compiler), visCPP) +CPP_DEF_FLAGS += -D_CRT_SECURE_NO_DEPRECATE +endif +ifeq ($(compiler), evc) CPP_DEF_FLAGS += -D_CRT_SECURE_NO_DEPRECATE endif

This code is in visCPP compiler section, under ifeq ($(compiler), visCPP), so it seems the first condition will always be true and the second will be false.

Danila

Hinkmond Wong wrote:

This is a call for external code review of a new code submission from Davy Preuveneers (4/24/2008).

His changes include changes to MIDP, Personal Basis/Personal Profile, and CLDC:

* The Alert patch has been extended a bit with a new method to play custom sounds (path is harded coded though), but the current behavior is to still play the default system sounds. This should provide some audio feedback if JSR 135 is not there.

* The AwtPPC and AwtPPCStub patches convert some path definitions with POSIX2HOST and add new missing method (clearBackground). It also provides a dummy PPCRobotHelper and related classes and sources to get the Personal Profile compilation working.

* The CLDC and MIDP patches currently only add configuration support for WM2003 (to compile with eVC4).

Here are the code diffs (vs. rev. 11040 of the phoneME SVN repo):

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/10/Alert.rev 11040.diff

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.re v11040.diff

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCStu bs.rev11040.diff

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev1 1040.diff

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev1 1040.diff

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/15/Suite.rev 11040.diff

Davy, please feel free to add any additional comments to this thread in the forums.

Everyone else, please take a look at the above diffs and send in your comments and/or questions on how they look for committing to our repository.

This code review will be open until Fri. 5/23/2008.

Thanks,

Hinkmond

--------------------------------------------------------------------- To unsubscribe, e-mail: feat@phoneme.dev.java.net For additional commands, e-mail: feat@phoneme.dev.java.net

-- Davy Preuveneers K.U.Leuven - http://www.kuleuven.be Department of Computer Science - http://www.cs.kuleuven.be DistriNet - http://www.cs.kuleuven.be/~distrinet Celestijnenlaan 200A, B-3001 Heverlee (Leuven), Belgium Room: 02.152 Phone: (+32) (0)16 327853 E-Mail: Davy@cs.kuleuven.be Web: http://www.cs.kuleuven.be/~davy