Basant thanks for the valuable comments. I believe that my patch is currently in sync with the current code convention. If there is sufficient interest in adopting a particular code convention, then I will definitely go ahead and adopt your suggestions. Looks like, memcache 2.2.3 is released since I originally posted this patch for review. thanks a ton for noticing this. I will integrate with memcache 2.2.3 after performing necessary validations
thanks sriram Basant Kukreja wrote: > Here are some of my comments : > --------------------------------------------------------- > 1. Also to me, in following line, $(MEMCACHE_DIR).tgz is not very unreadable. > +$(MEMCACHE_DIR)/configure: $(MEMCACHE_DIR).tgz > > I would rather define a variable e.g > MEMCACHE_TAR_FILE=memcache-$(MEMCACHE_VERSION).tgz > > and then write make rule like : > $(MEMCACHE_DIR)/configure: $(MEMCACHE_TAR_FILE) > gzip -dc $(MEMCACHE_TAR_FILE) | ... > -------------------------------------------------------------- > 2. Following two lines can be more concisely be written : > Current > + gzip -dc $(MEMCACHE_DIR).tgz | tar xopf - > + touch $(MEMCACHE_DIR)/configure > Suggested : > + gzip -dc $< | tar xopf - > + touch $@ > -------------------------------------------------------------- > 3. I would rather prefer to save configure options in a separate varaible e.g. > MEMCACHE_CONFIG_OPTION=--disable-debug \ > --enable-memcache \ > --with-php-config=../php-config-proto \ > --with-zlib-dir=$(ROOT)/usr ) > > $(MEMCACHE_DIR)/config.status: $(MEMCACHE_DIR)/configure > (cd $(MEMCACHE_DIR); \ > env $(ENVLINE) ../phpize-proto; \ > env $(ENVLINE) sh ./configure $(MEMCACHE_CONFIG_OPTION) > > -------------------------------------------------------------- > 4. There is small cosmetic change comments : > + env $(ENVLINE) ../phpize-proto; \ > + env $(ENVLINE) sh ./configure \ > + --disable-debug \ > + --enable-memcache \ > + --with-php-config=../php-config-proto \ > + --with-zlib-dir=$(ROOT)/usr ) > > > In above mentioned in first two lines, there is 1 tab, followed by 4 spaces, > while next four lines contain 2 tabs. I believe, we can make it more > consistent. > As far as Makefiles are concerned, we can safely use spaces after 1st tab. > > The issue is there in other places too in php5/Makefile.sfw. > --------------------------------------------------------- > > BTW memcache-2.2.3 version is also out. > http://pecl.php.net/package/memcache > > Regards, > Basant. > > > > On Fri, Feb 01, 2008 at 07:20:58PM -0800, Sriram Natarajan wrote: > >> Hi >> Can you kindly take a moment and review on the upcoming PHP memcached >> module integration related changes. Please find below the 'webrev' >> output and provide us with your valuable comments. Please note that >> 'memcached' server has been integrated with Nevada build 80. >> >> http://cr.opensolaris.org/~sn123202/6630059/webrev >> >> thanks in advance >> sriram >> _______________________________________________ >> >> >> webstack-discuss mailing list >> webstack-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >> > _______________________________________________ > > > webstack-discuss mailing list > webstack-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/webstack-discuss >
