Segher Boessenkool <seg...@kernel.crashing.org> writes:
> On Mon, Feb 03, 2020 at 01:24:04PM -0700, Jeff Law wrote:
>> ANd yes, even though I have been a regular ChangeLog user, I rely more
>> and more on the git log these days.
>
> As a reviewer, the changelog is priceless still. We shouldn't drop the
> changelog before people write *good* commit messages (and we are still
> quite far from that goal).
But that was the point of what others said upthread: now that the patch
description is part of the commit message, we should review them for
whether they're a good description. The patch description should be
as much a part of the review as the patch itself.
So IMO the right response to poor patch descriptions is to ask for
a better one. The goal should be "have good patch descriptions" not
"make all patch descriptions use format X". The traditional changelog
format should be one acceptable way of writing good patch descriptions,
but not the only acceptable way.
Sometimes the traditional changelog is better than a plain English
description (the SVE PCS work being one recent example from my POV).
But for modern VCSes there's IMO no benefit to a changelog like:
2017-09-05 Richard Sandiford <richard.sandif...@linaro.org>
* builtins.c (expand_builtin_powi): Use int_mode_for_size.
(get_builtin_sync_mode): Likewise.
(expand_ifn_atomic_compare_exchange): Likewise.
(expand_builtin_atomic_clear): Likewise.
(expand_builtin_atomic_test_and_set): Likewise.
(fold_builtin_atomic_always_lock_free): Likewise.
* calls.c (compute_argument_addresses): Likewise.
(emit_library_call_value_1): Likewise.
(store_one_arg): Likewise.
* combine.c (combine_instructions): Likewise.
* config/aarch64/aarch64.c (aarch64_function_value): Likewise.
* config/arm/arm.c (arm_function_value): Likewise.
(aapcs_allocate_return_reg): Likewise.
* config/c6x/c6x.c (c6x_expand_movmem): Likewise.
* config/i386/i386.c (construct_container): Likewise.
(ix86_gimplify_va_arg): Likewise.
(ix86_expand_sse_cmp): Likewise.
(emit_memmov): Likewise.
(emit_memset): Likewise.
(expand_small_movmem_or_setmem): Likewise.
(ix86_expand_pextr): Likewise.
(ix86_expand_pinsr): Likewise.
* config/lm32/lm32.c (lm32_block_move_inline): Likewise.
* config/microblaze/microblaze.c (microblaze_block_move_straight):
Likewise.
* config/mips/mips.c (mips_function_value_1) Likewise.
(mips_block_move_straight): Likewise.
(mips_expand_ins_as_unaligned_store): Likewise.
* config/powerpcspe/powerpcspe.c
(rs6000_darwin64_record_arg_advance_flush): Likewise.
(rs6000_darwin64_record_arg_flush): Likewise.
* config/rs6000/rs6000.c
(rs6000_darwin64_record_arg_advance_flush): Likewise.
(rs6000_darwin64_record_arg_flush): Likewise.
* config/sparc/sparc.c (sparc_function_arg_1): Likewise.
(sparc_function_value_1): Likewise.
* config/spu/spu.c (adjust_operand): Likewise.
(spu_emit_branch_or_set): Likewise.
(arith_immediate_p): Likewise.
* emit-rtl.c (gen_lowpart_common): Likewise.
* expr.c (expand_expr_real_1): Likewise.
* function.c (assign_parm_setup_block): Likewise.
* gimple-ssa-store-merging.c (encode_tree_to_bitpos): Likewise.
* reload1.c (alter_reg): Likewise.
* stor-layout.c (mode_for_vector): Likewise.
(layout_type): Likewise.
(choosing one of mine to avoid picking on someone else). I find
reviewing changelogs like this an extra burden rather than a help,
since reviewers are expected to make sure that every affected function
is listed, every sentence ends with a ".", the correct amount of spacing
is used, etc.
So I hope eventually we can be flexible and just use the changelog
format as one optional way of achieving a goal, rather than being
mandatory for every non-testsuite patch.
Richard
