Hi Yongchong,
After further review, I don't think that issue #2 is actually a
regression. Instead, I think it is a problem that has always been
there but was masked by the bug that you are fixing with the patch.
(There is some code merged from PathScale that looks related -- search
for -I in driver/OPTIONS -- but it is commented out.)
So please go ahead and check in the patch as you had it at this point,
plus the formatting changes to address issue #1. Thanks,
-David Coakley / AMD Open Source Compiler Engineering
On Tue, Jan 4, 2011 at 1:49 PM, David Coakley <dcoak...@gmail.com> wrote:
> 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
>>
>
------------------------------------------------------------------------------
Gaining the trust of online customers is vital for the success of any company
that requires sensitive data to be transmitted over the Web. Learn how to
best implement a security strategy that keeps consumers' information secure
and instills the confidence they need to proceed with transactions.
http://p.sf.net/sfu/oracle-sfdevnl
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
