Re: [PATCH v2 1/2] take comma expr in account for constant value

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

On Fri, Aug 4, 2017 at 11:05 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Fri, Aug 4, 2017 at 4:27 PM, Luc Van Oostenryck wrote:
> <luc.vanoostenryck@gmail.com> wrote:
>> I really prefer to keep independent things separated (for a lot of reasons)
>> but if it is a problme for you feel free to squash both together I don't mind
>> (and have no others series which would depend on this one).
>
> As I said, it is a minor one. I also realize it is some what personal
> preference.
> It will not impact how the patch get merged or not.

OK.

>>
>>> The test case on the other hand can justify to become a standalone patch.
>>> Some time we want see the how the code change (with or without patches)
>>> impact the test case.
>>
>> Well ... if you had the test case before the fix you break
>> bissectability and adding it after is pointless ...
>
> Before was the model for a while when Josh was the maintainer.
> I did not push separate test patch any more.
>
>> We could add it before and tagging it as known-to-fail
>> and remove the tag when adding the fix but ...
>
> May be not justifiable.
>
>>
>>> Another minor commend is that, if it is not for RC5.
>>
>> Well, it's up to you to decide if it is for -rc5 or not.
>
> I will listen to your recommendation as the base line.
>
> If it compress 20 seconds to 2 seconds. I vote for include it.

If only it would do so for every input code ;)

> But really that is your call too. I am fine either way.
>
>> Personally, I wouldn't take such a patch for such problem in -rc5
>> (but this decision would also depend on the frequency of releases,
>> for example). On one hand, the change seems quite limited and
>> 'obviously correct' (famous last words), on the other hand, every
>> changes, even the smallest need to retest things and delay this
>> release a bit more.
>
> After the RC5 release, we should have some good test on it any way.
> Chance of this patch breaking new things is relative low.

It can only impact code that is in fact erroneous.

>> Well, it's also because I would need to think a bit more about the API
>> look after the different uses regarding the warning flag and such.
>> I'm not even convinced that we must in general take in account
>> comma expressions like done here for this specific case.
>> All things that would need a bit more thinking and time.
>
> Yes, your call :-)

OK, considering also that the situation may be present in other
code (at various degree) and that this non-expansion of the
builtin is really bad I'm fine to include it.

Strangely I have a series of 5 or 6 patches fixing some similar situations
(where some code is not evaluated and/or expanded, often creating
duplicated warnings) but not this precise case.
I never posted them because I considered it was too late in the release ...

-- Luc