On Fri, Aug 4, 2017 at 11:05 PM, Christopher Li <sparse@xxxxxxxxxxx> wrote: > On Fri, Aug 4, 2017 at 4:27 PM, Luc Van Oostenryck wrote: > <luc.vanoostenryck@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
