All other changes in this patch look fine to me.
2010/10/11 David Coakley <dcoak...@gmail.com>
> 1. Yes, you are right, that is another way to fix the warnings. I did
> not fix it that way because I thought that mixing allocated data and
> constant data in the same array was bad style, worse than leaking a
> little more memory in a generator program.
>
> 2. I did not think C/C++ would allocate stack space for const data.
> In any case, I will revert that part of the change as it is not
> necessary to fix the warnings, which was my main goal.
>
> On Mon, Oct 11, 2010 at 12:49 PM, Jian-Xin Lai <laij...@gmail.com> wrote:
> > Hi David,
> >
> > Please refer my comments in below.
> >
> > 2010/10/11 David Coakley <dcoak...@gmail.com>
> >>
> >> Hi Jian-Xin,
> >>
> >> Thanks for looking at the changes.
> >>
> >> 1. Since sometimes the names are constructed using sprintf, I don't
> >> see how to make comp_name const. We could allocate fixed-length
> >> buffers if we want to avoid the strdup() and the leak with something
> >> like:
> >>
> >> static char comp_name[MAX_LISTING_OPERANDS][MAX_COMP_NAME_LEN];
> >
> > Convertion from char * (return type of strdup()) to const char * is safe.
> > Since we don't release the memory allocated by strdup, it looks your
> change
> > increases the memory leakage. The comp_name can be defined like:
> > static const char *comp_name[MAX_LISTING_OPERANDS];
> >
> >>
> >> 2. I thought the old declaration was confusing by being unusual:
> >>
> >> const char * const isa_exec_type_format = "...";
> >>
> >> My change is to use the common C idiom for string constants:
> >>
> >> const char isa_exec_type_format[] = "...";
> >>
> >> This form is preferred because it avoids introducing an unnecessary
> >> pointer.
> >
> > Since isa_exec_type_format is read only, we don't need to allocate space
> on
> > stack to hold the string. The second 'const' in original code ensures the
> > value of the pointer itself to be const.
> >
> >>
> >> -David Coakley / AMD Open Source Compiler Engineering
> >>
> >> On Mon, Oct 11, 2010 at 11:22 AM, Jian-Xin Lai <laij...@gmail.com>
> wrote:
> >> > Some comments:
> >> > 1. In osprey/common/targ_info/generate/isa_print_gen.cxx(line 188) and
> >> > osprey/common/targ_info/generate/isa_pack_gen.cxx (line 243):
> >> > I wonder it's better to change the type of comp_name from char* to
> const
> >> > char* so that we don't need to call strdup on string literal.
> >> >
> >> > 2. In osprey/common/targ_info/generate/isa_bundle_gen.cxx (line 349)
> >> > It looks like changing the type of isa_exec_type_format from pointer
> to
> >> > array is unnecessary.
> >> >
> >> > 2010/10/11 David Coakley <dcoak...@gmail.com>
> >> >>
> >> >> Hi Open64 gatekeepers,
> >> >>
> >> >> Has someone had a chance to review the attached cleanup patch?
> >> >>
> >> >> On Fri, Oct 1, 2010 at 4:26 PM, David Coakley <dcoak...@gmail.com>
> >> >> wrote:
> >> >> > Hi all,
> >> >> >
> >> >> > I am submitting a proposal to eliminate the scheduling info DSOs
> and
> >> >> > instead incorporate that info directly into the backend. The
> primary
> >> >> > motivation is portability. The proposal is described in more
> detail
> >> >> > in the attachment "no_si_dso.txt".
> >> >> >
> >> >> > Before making these changes, I thought it was a good idea to clean
> up
> >> >> > the compilation warnings in targ_info and its generated code, and
> to
> >> >> > fix some known portability problems (bugs 622, 624, 658). The
> patch
> >> >> > "ti_cleanup.diff" is a code cleanup patch that accomplishes those
> >> >> > goals without making any changes in the current behavior. The
> >> >> > details
> >> >> > are in the tentative log message, "log_msg.txt".
> >> >> >
> >> >> > Could a gatekeeper please review the patch?
> >> >> >
> >> >> > I have prototyped the proposed targ_info DSO changes for x8864. I
> >> >> > would like feedback from some of the gatekeepers and especially the
> >> >> > maintainers for other targets -- I will need your help since the
> >> >> > changes affect all targets. Thanks,
> >> >> >
> >> >> > -David Coakley / AMD Open Source Compiler Engineering
> >> >> >
> >> >>
> >> >>
> >> >>
> >> >>
> ------------------------------------------------------------------------------
> >> >> Beautiful is writing same markup. Internet Explorer 9 supports
> >> >> standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3.
> >> >> Spend less time writing and rewriting code and more time creating
> >> >> great
> >> >> experiences on the web. Be a part of the beta today.
> >> >> http://p.sf.net/sfu/beautyoftheweb
> >> >> _______________________________________________
> >> >> Open64-devel mailing list
> >> >> Open64-devel@lists.sourceforge.net
> >> >> https://lists.sourceforge.net/lists/listinfo/open64-devel
> >> >>
> >> >
> >> >
> >> >
> >> > --
> >> > Regards,
> >> > Lai Jian-Xin
> >> >
> >
> >
> >
> > --
> > Regards,
> > Lai Jian-Xin
> >
>
--
Regards,
Lai Jian-Xin
------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1, ECMAScript5, and DOM L2 & L3.
Spend less time writing and rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
