Hi Yongchong,
Sorry for the delay in the review and thanks for tracking down this
bug. I still see a few minor issues:
1) In the generated code, please follow the existing formatting
conventions: "char* path" should be "char *path" and "path=..." should
be "path = ..." (with spaces).
2) There seems to be a small regression when there is a trailing '-I'
in the command. Try:
$ opencc -I./libs test.c -o test -show -I
After your patch, you will see "-I./libs -I-default_options" passed to
the frontend. Previously, the "-I-default_options" was not there.
Note that gcc gives an error in this case, which I think is also ok:
$ gcc -I ./libs test.c -o test -I
gcc: argument to '-I' is missing
By the way, for any patch more than a few lines it is best to post it
as an attachment. The automatic formatting can make it difficult to
read and apply the patch.
-David Coakley / AMD Open Source Compiler Engineering
On Wed, Dec 29, 2010 at 10:40 PM, Wu Yongchong <wuyongch...@gmail.com> wrote:
> Here is another patch which do not use want_directory,
> this patch will remove the dead codes in table.c, and just use
> is_directory for generate a warning.
>
>
> Index: osprey/driver/table.c
> ===================================================================
> --- osprey/driver/table.c (revision 3448)
> +++ osprey/driver/table.c (working copy)
> @@ -752,6 +752,7 @@
> fprintf(f, "get_option(int *argi, char *argv[])\n");
> fprintf(f, "{\n");
> fprintf(f, "/* don't reset optargs, as may contain needed info */\n");
> + fprintf(f, "char* path;\n");
> fprintf(f, "optargd = 0;\n");
> fprintf(f, "switch (argv[*argi][optindex]) {\n");
> i = 0;
> @@ -827,7 +828,9 @@
> || options[i].syntax ==
> needs_directory_or_null
> #endif
> ) {
> - fprintf(f, "\tif
> (is_directory(next_string(argv,argi))) {\n");
> + fprintf(f, "\tpath=next_string(argv,argi);\n");
> + fprintf(f, "\tif (!is_directory(path) &&
> fullwarn)\n");
> + fprintf(f, "\t\twarning(\"%%s is not a
> directory\", path);\n");
> }
> fprintf(f, "\t\toptargs = get_optarg(argv, argi);\n");
> if (options[i].syntax == needs_decimal) {
> @@ -846,41 +849,14 @@
> fprintf(f, "\t\treturn
> add_string_option(%s,optargs);\n",
> options[i].flag);
> fprintf(f, "\t\t/* NOTREACHED */\n");
> - if (options[i].syntax == needs_directory
> -#ifdef KEY
> - || options[i].syntax == needs_directory_or_null
> -#endif
> - ) {
> - fprintf(f, "\t} else if
> (!is_last_char(argv,argi)) {\n");
> - fprintf(f, "\t\tif (fullwarn) {\n");
> - fprintf(f, "\t\t\twarning(\"%%s does
> not refer to a valid directory\", option_name);\n");
> - fprintf(f, "\t\t}\n");
> - fprintf(f, "\t\toptargs =
> get_optarg(argv,argi);\n");
> - fprintf(f, "\t\tget_next_arg(argi);\n");
> -#ifdef KEY
> - fprintf(f, "\t\treturn
> add_any_string_option(%s,optargs);\n",
> - options[i].flag);
> -#else
> - fprintf(f, "\t\treturn
> add_string_option(%s,optargs);\n",
> - options[i].flag);
> -#endif
> - fprintf(f, "\t\t/* NOTREACHED */\n");
> - }
> -#ifdef KEY
> - // Ignore %D? option if no dir arg is found by
> changing
> - // them into -dummy.
> - if (options[i].syntax == needs_directory_or_null) {
> - fprintf(f, "\t} else {\n");
> - fprintf(f, "\t optargs =
> current_string(argv,argi);\n");
> - fprintf(f, "\t get_next_arg(argi);\n");
> - fprintf(f, "\t return O_dummy;\n");
> - }
> -#endif
> if (options[i].syntax != needs_string
> && options[i].syntax != needs_string_or_dash
> + && options[i].syntax != needs_directory
> + && options[i].syntax != needs_directory_or_null
> ) {
> fprintf(f, "\t}\n");
> }
> +
> if (strlen(options[i].name) > 2) {
> fprintf(f, "\t}\n");
> }
>
>
> On Thu, Dec 30, 2010 at 10:42 AM, David Coakley <dcoak...@gmail.com> wrote:
>> I will not have a chance to look closely until next week, but I have a
>> few minor complaints... we can always address these later if the bug
>> is blocking progress.
>>
>> The comment for want_directory() is exactly the same as the comment
>> for is_directory(). This looks like an error.
>>
>> Also, I think it is strange to have a boolean function that always
>> returns TRUE. Maybe the code generated by table.c should change
>> instead.
>>
>> -David Coakley / AMD Open Source Compiler Engineering
>>
>> On Wed, Dec 29, 2010 at 3:18 PM, Sun Chan <sun.c...@gmail.com> wrote:
>>> go ahead.
>>> Sun
>>>
>>> On Tue, Dec 28, 2010 at 11:31 PM, Wu Yongchong <wuyongch...@gmail.com>
>>> wrote:
>>>> Hi, David
>>>>
>>>> Could you please help review this patch
>>>> See https://bugs.open64.net/show_bug.cgi?id=709
>>>>
>>>> In revision 3437 , the function want_directory has been remove
>>>> considering it's a dead function, but it's not true. This function
>>>> will always return TRUE no matter the path is a directory or not.
>>>> If it's replace with is_directory, the following situation will fail .
>>>>
>>>> There is no directory named libs
>>>> $ opencc -I ./libs test.c
>>>> opencc WARNING: unknown flag: -I
>>>> gcc: ./libs: No such file or directory
>>>>
>>>>
>>>> Here is the patch.
>>>> Index: osprey/driver/file_utils.c
>>>> ===================================================================
>>>> --- osprey/driver/file_utils.c (revision 3437)
>>>> +++ osprey/driver/file_utils.c (working copy)
>>>> @@ -163,6 +163,13 @@
>>>> return FALSE;
>>>> }
>>>>
>>>> +boolean want_directory (char *path)
>>>> +{
>>>> + if (is_directory(path) && fullwarn)
>>>> + warning("%s is not a directory", path);
>>>> + return TRUE;
>>>> +}
>>>> +
>>>> /* check if directory is writable */
>>>> boolean
>>>> directory_is_writable (char *path)
>>>> Index: osprey/driver/file_utils.h
>>>> ===================================================================
>>>> --- osprey/driver/file_utils.h (revision 3437)
>>>> +++ osprey/driver/file_utils.h (working copy)
>>>> @@ -61,6 +61,9 @@
>>>> /* check whether is a directory */
>>>> extern boolean is_directory (char *path);
>>>>
>>>> +/* check whether is a directory */
>>>> +extern boolean want_directory (char *path);
>>>> +
>>>> /* check whether directory is writable */
>>>> extern boolean directory_is_writable (char *path);
>>>>
>>>> Index: osprey/driver/table.c
>>>> ===================================================================
>>>> --- osprey/driver/table.c (revision 3437)
>>>> +++ osprey/driver/table.c (working copy)
>>>> @@ -827,7 +827,7 @@
>>>> || options[i].syntax ==
>>>> needs_directory_or_null
>>>> #endif
>>>> ) {
>>>> - fprintf(f, "\tif
>>>> (is_directory(next_string(argv,argi))) {\n");
>>>> + fprintf(f, "\tif
>>>> (want_directory(next_string(argv,argi))) {\n");
>>>> }
>>>> fprintf(f, "\t\toptargs = get_optarg(argv,
>>>> argi);\n");
>>>> if (options[i].syntax == needs_decimal) {
>>>>
>>>>
>>>> --
>>>> yongchong
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Learn how Oracle Real Application Clusters (RAC) One Node allows customers
>>>> to consolidate database storage, standardize their database environment,
>>>> and,
>>>> should the need arise, upgrade to a full multi-node Oracle RAC database
>>>> without downtime or disruption
>>>> http://p.sf.net/sfu/oracle-sfdevnl
>>>> _______________________________________________
>>>> Open64-devel mailing list
>>>> Open64-devel@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/open64-devel
>>>>
>>>
>>
>
>
>
> --
> yongchong
>
------------------------------------------------------------------------------
Learn how Oracle Real Application Clusters (RAC) One Node allows customers
to consolidate database storage, standardize their database environment, and,
should the need arise, upgrade to a full multi-node Oracle RAC database
without downtime or disruption
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
