On 6/8/20 7:07 PM, Asher Gordon wrote:
Hi Martin,
Thanks for your helpful comments.
Martin Sebor <mse...@gmail.com> writes:
As an aside, the bug number should be referenced in the change
message so that when the patch is committed it's automatically
reflected in the bug entry. The contrib/mklog.py script should
do that automatically based on new test files added to verify
the bug fix/change, provided one such test is added and starts
with a PR c/95379 comment. Also mentioning it in the Subject
of the email is helpful.
[...]
I don't think there's any "rule" against it but it's customary
to add test files for new features, rather than modify existing
ones (see also my comment about the post-commit hook above).
[...]
ChangeLog changes need not be included in a patch (and shouldn't
be). They are now automatically extracted from the commit message
and added to the ChangeLog files by a post-commit hook.
OK, these will be easy to fix. Should my other patch¹ (which implements
-Wuniversal-initializer) also have PR c/95379?
I'm sure that would be fine but since it's a separate enhancement
it doesn't need to. It's up to you.
Rather than creating own linked list I suggest to consider making
use of one of GCC's data structures.
How would this be done? I read the gccint manual a bit and, from what I
understand, a 'tree' type can be a linked list of other 'tree's. But how
would I make a linked list of generic types ('location_t's)?
I don't think GCC has its own generic linked list container.
There's TREE_CHAIN of tree nodes, but that's probably not
the best choice for this purpose. I think the vec template
(defined in <vec.h>) would come closest to it (like
constructor_stack::elements).
GCC has its own garbage collector so it's preferable to avoid
manual memory management and all its pitfalls (plus, it's much
more fun to learn about those unique to the garbage collector ;-)
Martin
If the argument to free was returned from XNEW it should probably
be released by XDELETE (even if the two do the same same thing).
In that case, I believe that all other instances of free (which free
something allocated by either XNEW or XRESIZEVEC) should be changed to
XDELETE. The following patch does that (created against the latest
master, without my patches applied).
I realize the patch doesn't change the text of the message but...
if the name of the field is known at this point, mentioning it in
the message would be a helpful touch. If it isn't, then "a field"
would be grammatically more correct. In addition, pointing to
the definition of the struct in a follow-on note would also be
helpful (and in line with other similar messages). These
improvements are unrelated to the change you're making so could
be made separately, but since you're already making changes here
it's an opportunity to make them now (otherwise they're unlikely
to happen).
I will look into this.
Thanks,
Asher
Footnotes:
¹ https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547460.html
