Re: [PATCH 2/3] xdiff: -W: include immediately preceding non-empty lines in context
- From
René Scharfe <l.s.r@web.de>
- Date
- Jan 15, 2017, 16:57 UTC
- Message-ID
- <2668771d-249b-659d-3a2c-a788d7d5ebd6@web.de>
- In-Reply-To
- <0c761135-2696-4b3d-0a4f-3d90edf5da2e@oracle.com>
Am 15.01.2017 um 11:06 schrieb Vegard Nossum:
Show 39 quoted lines
> On 15/01/2017 03:39, Junio C Hamano wrote: >> René Scharfe <l.s.r@web.de> writes: >>> How about extending the context upward only up to and excluding a line >>> that is either empty *or* a function line? That would limit the extra >>> context to a single function in the worst case. >>> >>> Reducing context at the bottom with the aim to remove comments for the >>> next section is more tricky as it could remove part of the function >>> that we'd like to show if we get the boundary wrong. How bad would it >>> be to keep the southern border unchanged? >> >> I personally do not think there is any robust heuristic other than >> Vegard's "a blank line may be a signal enough that lines before that >> are not part of the beginning of the function", and I think your >> "hence we look for a blank line but if there is a line that matches >> the function header, stop there as we know we came too far back" >> will be a good-enough safety measure. >> >> I also agree with you that we probably do not want to futz with the >> southern border. > > You are right, trying to change the southern border in this way is not > quite reliable if there are no empty lines whatsoever and can > erroneously cause the function context to not include the bottom of the > function being changed. > > I'm splitting the function boundary detection logic into separate > functions and trying to solve the above case without breaking the tests > (and adding a new test for the above case too). > > I'll see if I can additionally provide some toggles (flags or config > variables) to control the new behaviour, what I had in mind was: > > -W[=preamble,=no-preamble] > --function-context[=preamble,=no-preamble] > diff.functionContextPreamble = <bool> > > (where the new logic is controlled by the new config variable and > overridden by the presence of =preamble or =no-preamble).
Adding comments before a function is useful, removing comments after a function sounds to me as only nice to have (under the assumption that they belong to the next function[*]). How bad would it be to only implement the first part (as in the patch I just sent) without adding new config settings or parameters?
Thanks, René
[*] Silly counter-example (the #endif line):
#ifdef SOMETHING
int f(...) {
// implementation for SOMETHING
}
#else
inf f(...) {
// implementation without SOMETHING
}
#endif /* SOMETHING */