grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: merging of gettext branch


From: Carles Pina i Estany
Subject: Re: merging of gettext branch
Date: Sun, 22 Nov 2009 12:46:32 +0000
User-agent: Mutt/1.5.20 (2009-06-14)

Hello,

Briefly: applied the suggested changes, and new patch attached. Don't
hesitate to say anything back (I know that you don't :-) ). Later I will
spend some time with it again.

Cheers,

On Nov/22/2009, Robert Millan wrote:
> On Sat, Nov 21, 2009 at 10:14:10PM +0000, Carles Pina i Estany wrote:
> > @@ -179,7 +185,7 @@
> >  pkglib_MODULES += fshelp.mod fat.mod ufs1.mod ufs2.mod ext2.mod ntfs.mod \
> >     ntfscomp.mod minix.mod hfs.mod jfs.mod iso9660.mod xfs.mod      \
> >     affs.mod sfs.mod hfsplus.mod reiserfs.mod cpio.mod tar.mod      \
> > -   udf.mod afs.mod afs_be.mod befs.mod befs_be.mod
> > +   udf.mod afs.mod afs_be.mod befs.mod befs_be.mod gettext.mod
> >  
> >  # For fshelp.mod.
> >  fshelp_mod_SOURCES = fs/fshelp.c
> > @@ -610,6 +616,11 @@
> >  bufio_mod_CFLAGS = $(COMMON_CFLAGS)
> >  bufio_mod_LDFLAGS = $(COMMON_LDFLAGS)
> >  
> > +# For gettext.mod.
> > +gettext_mod_SOURCES = gettext/gettext.c
> > +gettext_mod_CFLAGS = $(COMMON_CFLAGS)
> > +gettext_mod_LDFLAGS = $(COMMON_LDFLAGS)
> 
> I haven't switched the old declarations, but for new modules please use
> something like:
> 
> pkglib_MODULES += gettext.mod
> gettext_mod_SOURCES = gettext/gettext.c
> gettext_mod_CFLAGS = $(COMMON_CFLAGS)
> gettext_mod_LDFLAGS = $(COMMON_LDFLAGS)

Done!

> this way it is easier to move declarations around, etc.
> 
> > +static grub_file_t grub_mofile_open (const char *name);
> 
> We usually skip declarations for static functions (when all its users are
> placed after its implementation, of course).

Done!

> > +#define GETTEXT_MAGIC_NUMBER 0
> > +#define GETTEXT_FILE_FORMAT 4
> > +#define GETTEXT_NUMBER_OF_STRINGS 8
> > +#define GETTEXT_OFFSET_ORIGINAL 12
> > +#define GETTEXT_OFFSET_TRANSLATION 16
> 
> These would look much prettier with some tabs ;-)

Done!

> > +  grub_file_seek (fd_mo, offset);
> > +  grub_file_read (fd_mo, translation, length);
> 
> You do a lot of grub_file_seek + grub_file_read throurough the code.  Maybe
> grub_file_pread() would be more appropiate?  (for space saving)

Done!

> > +          grub_free(current_string);
> > +          min=current;
> > [...]
> > +    current = (max+min)/2;
> > [...]
> > +  ret = grub_malloc(grub_strlen(orig) + 1);
> > +  grub_strcpy(ret,orig);
> 
> Missing spaces (between function name and parenthesis, around '=', etc).
> 
> I suggest you use indent(1), this will automatically adjust to our coding
> style.

used indent, Done!

> > +  /*
> > +  Do we want .mo.gz files? Then, the code:
> > +  file = grub_gzio_open (io, 0); // 0: transparent
> > +  if (! file)
> > +    {
> > +      grub_printf("Problems opening the file\n");
> > +      grub_file_close (io);
> > +      return 0;
> > +    }
> > +  */
> 
> Uhm I wonder if we could answer this question before the code is merged; what
> does everyone think about .mo.gz?
> 
> > +  locale_dir = grub_env_get ("locale_dir");
> 
> This needs to be checked for NULL.

Why? If it's NULL it will try to open a .mo file that doesn't exist
(probably) and will print the path. So the user will notice that
somethiing is missing.

I've added a check, grub_printf saying that the variable is not setted
up and a return but not sure if this should happen.

> > +  // mo_file e.g.: /boot/grub/locale/ca.mo
> 
> Please use C-style comments (/* */).

Done!

> > +  mo_file = grub_malloc (grub_strlen (locale_dir) + sizeof ("/") + 
> > grub_strlen (lang) + sizeof(".mo"));
> 
> Note that sizeof ("/") equals 2 ('/' + '\0').

changed "/" by '/', you are right. Done!

> > +  grub_dprintf("gettext", "Will try to open file: %s " ,mo_file);
> > +
> > +  fd_mo = grub_mofile_open(mo_file);
> 
> In this case I think error handling would be more useful than debug
> print (i.e.  if file can't be opened, rise an error).

I don't remember the details, but if grub_mofile_open cannot open it it
raise an error:

  fd_mo = grub_file_open (filename);
  if (!fd_mo)
    {
      grub_error (GRUB_ERR_FILE_READ_ERROR, "Cannot read %s", filename);
      return 0;
    }

and the caller will not do anything if it returns 0. 


> > +  /* Testing:
> > +  grub_register_command ("_", grub_cmd_translate, GRUB_COMMAND_FLAG_BOTH,
> > +                    "_", "internalization support trans", 0);
> > +  */
> 
> We could define this interface unconditionally, but it'd be more consistent
> as `gettext' command (like the one in GNU gettext package).

We didn't include anything to not populate the standard shell with
superflous commands. By the moment I've removed above lines, we can add it
later.

> 
> > === added file 'include/grub/i18n_grub.h'
> > --- include/grub/i18n_grub.h        1970-01-01 00:00:00 +0000
> > +++ include/grub/i18n_grub.h        2009-11-19 21:32:05 +0000
> > [...]
> > +#ifndef    GRUB_I18N_GRUB_H
> > +#define    GRUB_I18N_GRUB_H        1
> > +
> > +# define _(str) grub_gettext(str)
> > +
> > +#endif /* GRUB_I18N_GRUB_H */
> 
> You can use <grub/i18n.h> for this (in fact it already defines _ to a
> noop stub for non-util and would just need to be modified).

Changed, I didn't read it properly some days ago. So... Done!

> > +const char *EXPORT_FUNC(grub_gettext_dummy) (const char *s);
> > +extern const char *(*EXPORT_VAR(grub_gettext)) (const char *s);// = 
> > grub_gettext_dummy;
> 
> This could be in i18n.h too (except the comment ;-)).
> 
> Btw, is it necessary to export grub_gettext_dummy symbol?  If we avoid it
> it's a few less bytes in kernel :-)

if it's not exported it doesn't compile:
kern/misc.c:33: error: 'grub_gettext_dummy' undeclared here (not in a function)

> > +# Gettext variables and module
> > +cat << EOF
> > +set locale_dir=${locale_dir}
> > +set lang=${grub_lang}
> > +insmod gettext 
> > +EOF
> 
> We could avoid loading the module at all for non-utf8 capable setups (when
> this happens, grub-mkconfig exports LANG=C, so this can be easily detected).

Done!

-- 
Carles Pina i Estany
        http://pinux.info

Attachment: gettext08.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]