Re: /usr.bin/colldef and collation tables
On Tue, Mar 26, 2013 at 07:31:01AM -0500, Vladimir Támara Patiño wrote: > I didnt' receive answer if I should send both patches as one. Since > I corrected a realloc in collate.c, here I'm sending everything > (including man pages of wcscoll, wcsxfrm and corrections > for strcoll and strxfmr). I don't have much time right now to read this in detail and try it out. But I've skimmed it once more and wanted to give you some feedback. The style of the lex/yacc stuff in colldef looks good to me now. It seems you have an outdated version of colldef.h in this diff, e.g. the STR_LEN define is back. I doubt this diff compiles as-is. It seems there is some compat stuff in collate.c, at least that what this constant seems to be used for: > +#define COLLATE_VERSION"1.1\n" Is this really needed? Can we remove this, or is your diff adding colldef files which use the 1.1 format? wcscoll.c has indentation issues (not very important but it would be good to make this code KNF, see the style(9) man page). There are several C++ comments in this patch (// foo), please change them to /* foo */ or remove them if not strictly necessary. As discussed before, comments like this one aren't necessary: /** In OpenBSD using wchar_t, while the original from FreeBSD was char * */ I'd remove any references to xlocale from this diff. It really doesn't matter right now, this diff should focus on collations support only. For instance, I'd rename the struct xlocale_collate to something like struct __collate.
Re: /usr.bin/colldef and collation tables
On Sat, Mar 23, 2013 at 04:09:12PM +0100, Stefan Sperling wrote: On Sat, Mar 23, 2013 at 09:27:50AM -0500, Vladimir Támara Patiño wrote: > >Please change this to look like > >in other parse.y files in the OpenBSD src tree (e.g. look at bgpd). > > Could you change? No, sorry. I don't have time for doing that. My time is also limited. Please cross-check the parse.y files yourself and make sure the new parse.y file follows conventions used in the existing parse.y files. We want our source tree to be consistent. I think it is somehow similar to lex.l and yacc.y of usr.bin/mklocale But, however I would like to know what conventions of other parse.y files are desirable (even for mklocale)? * Using log.c ? * Not using lex, but implementing the lexer in parse.y? I see you're still tempted to copy code from FreeBSD without making it perfect for OpenBSD. I'm not going to allow you to be that lazy ;-) I made small changes, like improving documentation, using strlcpy and strlcat, adding debugging of lexer when the option -d is used. Comparing mklocale current implementations with NetBSD's, I see improving documentation and renamings. What other conventions are important? Possible to show example with mklocale's lexer and parser? Why do you compile colldef with COLLAGE_DEBUG by default? To have the option -d that gives debuggin information (mklocale also has it). Don't expect me to hunt for additional opinions and OKs before I've gotten around to review the libc changes you sent as well, and before I'm perfectly happy with both diffs. Ok. Better then sending both parts as one patch? I'm certainly not going to commit any of this without asking others for review as well, so please don't be worried if it takes some time to get this in. Ok. collate.h has this: This should be named something like COLLATE_MAX_STR_LEN. Ok. Also, the collate.h file has: > +__BEGIN_DECLS > +#ifdef COLLATE_DEBUG > +void __collate_print_tables(void); This is bogus. colldef.c declares and defines a collate_print_tables() function. The __collate_print_tables() function is used in the libc diff you sent, so it doesn't belong in the colldef diff. Ok. Should I sent now, only one diff (with changes for colldef, collation tables and changes to libc ?) -- Dios, gracias por tu amor infinito. -- Vladimir Támara Patiño. http://vtamara.pasosdeJesus.org/ http://www.pasosdejesus.org/dominio_publico_colombia.html
Re: /usr.bin/colldef and collation tables
On Sat, Mar 23, 2013 at 09:27:50AM -0500, Vladimir Támara Patiño wrote: > >Please change this to look like > >in other parse.y files in the OpenBSD src tree (e.g. look at bgpd). > > Could you change? No, sorry. I don't have time for doing that. Please cross-check the parse.y files yourself and make sure the new parse.y file follows conventions used in the existing parse.y files. We want our source tree to be consistent. I see you're still tempted to copy code from FreeBSD without making it perfect for OpenBSD. I'm not going to allow you to be that lazy ;-) > The attached patch only adds LC_COLLATE for existing locales (single > byte or translatable to ISO8859-1). The new diff looks much better already. Why do you compile colldef with COLLAGE_DEBUG by default? > In case you want to give me credit in the CVS log, could you please use > my email? Sure. But I don't know yet if/how this is going to be committed. I think it makes sense to put the libc bits and usr.bin parts in at the same time. Don't expect me to hunt for additional opinions and OKs before I've gotten around to review the libc changes you sent as well, and before I'm perfectly happy with both diffs. I'm certainly not going to commit any of this without asking others for review as well, so please don't be worried if it takes some time to get this in. I was working on and off on UTF-8 locale support over the course of two years -- these things can take some time, and I'm not going to rush things unnecessarily. But I think you're on the right track. collate.h has this: > +#define STR_LEN 10 which is a very bad generic name. This should be named something like COLLATE_MAX_STR_LEN. Also, the collate.h file has: > +__BEGIN_DECLS > +#ifdef COLLATE_DEBUG > +void __collate_print_tables(void); This is bogus. colldef.c declares and defines a collate_print_tables() function. The __collate_print_tables() function is used in the libc diff you sent, so it doesn't belong in the colldef diff. > +#endif > +__END_DECLS > + > +#endif /* !_COLLATE_H_ */ >
Re: /usr.bin/colldef and collation tables
On Fri, Mar 22, 2013 at 05:56:50AM -0500, Vladimir Támara Patiño wrote: > I reworked collation support based on: > http://marc.info/?l=openbsd-tech&m=136283674512235&w=4 > > Here I'm attaching a first part consisting of: > > * The program colldef based on FreeBSD (similar to mklocale that is > based on NetBSD). This programa generates files LC_COLLATE good > only for single-byte encondings. > * Collation tables based on FreeBSD (similar to ctype case that is > based on NetBSD) for all the single-byte locales that OpenBSD > currently support (and some other that could be added or ignored). > * Header lib/libc/locale/collate.h required by colldef and for next > part of collation support. Thanks! I've reviewed this but haven't tried running it yet. It looks like a good starting point. The colldef.1 man page contains typos. And it doesn't pass mandoc -Tlint and that alone would break the build. realloc() calls in parse.y violate best practice rules explained in the malloc(3) man page. It seems parse.y also contains some macros to support compilers other than gcc, I don't think it's worth adding these yet. The __printflike() macro should be dropped, it is used to redefine yyerror(). Please change this to look like in other parse.y files in the OpenBSD src tree (e.g. look at bgpd). The parse.y file also contains some indentation style issues, but those can be fixed later. The file share/locale/colldef/Makefile contains comments that explain differences to FreeBSD. Such comments make some sense in email you send to tech@ but they don't need to be in the source code. The file also lists locales which do not have corresponding ctype locale defintions yet. Please do not add them to LC_COLLATE just yet. We can add them later if needed. For now they just clutter your diff and add additional files to the installation sets, which is something we should discuss separately (see the other tech@ discussion about your ctype Makefile diff). Adding new locales and adding support for LC_COLLATE are difference concerns and should be done in separate diffs in my opinion. The collate.h files seems to contain definitions not used by colldef. Can we please start out with a header file that only contains things which are actually used?
