Message ID | 20100708061704.GA8748@intrepid.com |
---|---|
State | New |
Headers | show |
>>>>> "Gary" == Gary Funck <gary@intrepid.com> writes:
Hi.
I didn't really read most of the patch, but this bit stood out for me:
Gary> --- gcc/tree.h (.../trunk) (revision 161517)
Gary> +++ gcc/tree.h (.../branches/gupc) (revision 161914)
Gary> @@ -367,6 +367,12 @@ struct GTY(()) tree_base {
Gary> unsigned asm_written_flag: 1;
Gary> unsigned nowarning_flag : 1;
Gary> + /* UPC flags */
Gary> + unsigned shared_flag : 1; /* UPC: shared qualified */
Gary> + unsigned strict_flag : 1; /* UPC: strict qualified */
Gary> + unsigned relaxed_flag : 1; /* UPC: relaxed qualified */
Gary> + unsigned upc_unused : 5; /* UPC: unused bits */
I think it would be better to use some bits from tree_base's "spare"
field and not have "upc_unused" at all. tree_base is size-sensitive.
Tom
On Wed, 7 Jul 2010, Gary Funck wrote: > The UPC-related front-end "diff's" are attached > for review. All feedback and suggestions This patch does not seem to be in a state in which it is ready for detailed technical review of whether it implements correct semantics. * Before posting a patch for review, clean up FIXMEs and TODOs. * There are a large number of copyright and license notices in an extremely obsolete form, referencing "GNU CC". Make sure to use the current standard forms of all such notices. * Review code for correct conformance to coding standards. Comments before all functions that explain the function semantics, all parameters and the return value. Spaces before parentheses where appropriate. Diagnostics following GNU standards including no closing "." and starting with lowercase. * Do not try to build up diagnostics with sprintf; this is unfriendly to i18n. Put appropriate format strings directly in the calls to diagnostic functions. Avoid direct use of IDENTIFIER_POINTER in diagnostics as that is also unfriendly to i18n; use formats such as %qD instead. * It is not acceptable for files in c-family/ to include c-tree.h, because c-tree.h describes facilities only available in the C and ObjC front ends and not available for C++. Do not revert recent modularity improvements such as this. * It appears you have a large number of #ifdef conditionals with no obvious reason for the macros being conditionally defined or not defined. The use of conditional compilation like this is deprecated for new code. If something may vary from target to target, use target hooks, not macros, and document them in tm.texi (if they are in fact documented there in something outside this patch, perhaps you need to post that other patch). Conditions using "if" are strongly preferred to those using "#if" whenever possible. * In general, since this code has evidently been around for a very long time given such things as use of "GNU CC", it needs careful review for whether it accords with current GCC coding practices for new code and takes account of cleanups done in the past decade or so, rather than with the practices of a decade ago. * Make sure the diffs do not make unrelated changes to code not otherwise being modified. For example, the @@ -5387,7 +5471,7 @@ grokdeclarator (const struct c_declarato diff hunk appears just to be changing the indentation of a comment, and while that looks like a correct change it's got nothing to do with UPC, so should go in trunk separately.
On 07/27/10 22:39:50, Joseph S. Myers wrote: > On Wed, 7 Jul 2010, Gary Funck wrote: > > The UPC-related front-end "diff's" are attached > > for review. All feedback and suggestions > > This patch does not seem to be in a state in which it is ready for > detailed technical review of whether it implements correct semantics. [...] Joseph, thanks for the detailed review and suggestions. I will re-work the GUPC front-end related changes into the appropriate form, and re-submit the patch. - Gary
--- gcc/tree.h (.../trunk) (revision 161517) +++ gcc/tree.h (.../branches/gupc) (revision 161914) @@ -367,6 +367,12 @@ struct GTY(()) tree_base { unsigned asm_written_flag: 1; unsigned nowarning_flag : 1; + /* UPC flags */ + unsigned shared_flag : 1; /* UPC: shared qualified */ + unsigned strict_flag : 1; /* UPC: strict qualified */ + unsigned relaxed_flag : 1; /* UPC: relaxed qualified */ + unsigned upc_unused : 5; /* UPC: unused bits */ --- gcc/c-tree.h (.../trunk) (revision 161517) +++ gcc/c-tree.h (.../branches/gupc) (revision 161914) @@ -221,6 +261,9 @@ struct c_declspecs { NULL; attributes (possibly from multiple lists) will be passed