Re: [PATCHv2] Fix setting breakpoints or stepping on line 65535

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi <simark@simark.ca> [2019-12-29 14:45:13 -0500]:

> On 2019-12-02 1:04 p.m., Bernd Edlinger wrote:
> > Hi Andrew,
> > 
> > On 12/1/19 11:08 PM, Andrew Burgess wrote:
> >> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2019-11-24 11:54:23 +0000]:
> >>
> >>> Hi,
> >>>
> >>> this removes code that is present from the very first git revisison
> >>> 7b4ac7e1ed2c4616bce56d1760807798be87ac9e from 1988.  It was in the
> >>> gdb/dbxread.c at the time (and makes more sense for dbx line info format
> >>> since line numbers are 16-bit entities in that debug format and debugging
> >>> files with more than 65535 lines would not work anyway) but moved from
> >>> there to gdb/buildsym.c which is used for dwarf line info as well, and
> >>> excluding an arbitrary line number does certainly not make sense nowadays.
> >>>
> >>>
> >>> Thanks
> >>> Bernd.
> >>
> >>> From f202ae765b72ad6d17600eb661993a63191309f7 Mon Sep 17 00:00:00 2001
> >>> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
> >>> Date: Sat, 23 Nov 2019 07:37:26 +0100
> >>> Subject: [PATCH 1/2] Fix setting breakpoints or stepping on line 65535
> >>>
> >>
> >> Bernd,
> >>
> >> Thanks for looking into this, and especially thanks for adding a test!
> >>
> >> Normally you should include the git commit message and ChangeLog along
> >> with your patch submission so that these can be reviewed too. 'git
> >> format-patch' and 'git send-email' can be useful for this, if you can
> >> get them setup.
> >>
> > 
> > Okay, I added changelog messages for gdb/ChangeLog and gdb/testsuite/ChangeLog,
> > and improved the commit messages.
> > I hope you can handle the format-patch files as attachments. 
> > 
> >> Given the age of the code you're removing I think this change sounds
> >> reasonable.  I assume there's no test that covers why this code should
> >> be there, so you see no regressions with this code removed?
> >>
> > 
> > No, there were no regressions (last full test on 24-11-19).
> > 
> >> I have a couple of minor issues with the test.  If you address those
> >> and repost with commit message and ChangeLog this can be approved.
> >>
> > 
> > Fixed the test.
> > 
> > Thanks a lot for your review.
> > Is it OK for trunk?
> 
> Thanks Bernd, this is OK.  For the future, I think it would be reasonable to put the
> fix and the test in the same patch.  It would also help somebody wondering why we have
> such a weird test trace it back to the fix, and the explanation you have put in the first
> patch.  But it's also fine like this.

I place more value on having tests and changes in the same commit - I
find it more useful the other way around, when I find some code I
don't understand and I figure out which commit it came from, having
the tests in the same commit means it is easy to know "this test"
should check "this code".

If the test and changes are separate I have to do more work. Boo!

For this reason, if you've not already pushed this patch, could you
collapse the test and change into a single commit please before
pushing.

Thanks,
Andrew