On Fri, Aug 4, 2017 at 9:49 PM, Christopher Li <sparse@xxxxxxxxxxx> wrote: > Looks fine to me. > > One of the very minor note is that. I think this patch can actually merge with > the next one. It is easier to read patch have both the define and usage. > I found myself need to switch between patches to read how it was used. 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). > 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 ... We could add it before and tagging it as known-to-fail and remove the tag when adding the fix but ... > 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. 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. > You actually don't need to worry about code impact range > any more. You might just as well do the full change into the constant > expression value evaluation. 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. -- 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
