| Message ID | 20170804160622.18260-2-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
On Fri, Aug 4, 2017 at 12:06 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > It's often the case that we simply need the expr's truth value. > > To get the value of an expression or simply if we need to know > if it's a constant value we can use some functions like > get_expression_value() or const_expression_value() depending > on the type of warning needed if the expression is not constant > (for various definition of constant). > > However none of these functions take in account the fact that > a comma expression can also have a value known at compile time. > > In order to not introduce unwanted change elsewheer in the code, > introduce a new function expr_truth_value() which never warn > but will return -1 if the expr have no known boolean value. > > Note: the whole set of functions should be unified to take comma > expressions in account when needed. > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > --- > expand.c | 30 ++++++++++++++++++++++++++++++ > expression.h | 1 + > 2 files changed, 31 insertions(+) > > diff --git a/expand.c b/expand.c > index 5f908c971..f436b5b50 100644 > --- a/expand.c > +++ b/expand.c > @@ -1281,6 +1281,36 @@ long long get_expression_value_silent(struct expression *expr) > return __get_expression_value(expr, 2); > } > > +int expr_truth_value(struct expression *expr) 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. 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. Another minor commend is that, if it is not for RC5. 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. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 9:49 PM, Christopher Li <sparse@chrisli.org> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 4:27 PM, Luc Van Oostenryck <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. > >> 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. 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. > 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 :-) Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 4, 2017 at 5:20 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: >> >> If it compress 20 seconds to 2 seconds. I vote for include it. > > If only it would do so for every input code ;) > Because the offending source is come from a system header file, I think it is more likely to hit in the field than average project source file. > > 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 ... I already applied as patch to sparse-next. I am doing the kernel compile test right now. I will push it soon. I confirm it cut the offending compile from 23 secons to: real 0m3.425s user 0m3.237s sys 0m0.135s Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/expand.c b/expand.c index 5f908c971..f436b5b50 100644 --- a/expand.c +++ b/expand.c @@ -1281,6 +1281,36 @@ long long get_expression_value_silent(struct expression *expr) return __get_expression_value(expr, 2); } +int expr_truth_value(struct expression *expr) +{ + const int saved = conservative; + struct symbol *ctype; + + if (!expr) + return 0; + + ctype = evaluate_expression(expr); + if (!ctype) + return -1; + + conservative = 1; + expand_expression(expr); + conservative = saved; + +redo: + switch (expr->type) { + case EXPR_COMMA: + expr = expr->right; + goto redo; + case EXPR_VALUE: + return expr->value != 0; + case EXPR_FVALUE: + return expr->fvalue != 0; + default: + return -1; + } +} + int is_zero_constant(struct expression *expr) { const int saved = conservative; diff --git a/expression.h b/expression.h index 80b3be5f5..d0f3ac925 100644 --- a/expression.h +++ b/expression.h @@ -178,6 +178,7 @@ struct expression { /* Constant expression values */ int is_zero_constant(struct expression *); +int expr_truth_value(struct expression *expr); long long get_expression_value(struct expression *); long long const_expression_value(struct expression *); long long get_expression_value_silent(struct expression *expr);
It's often the case that we simply need the expr's truth value. To get the value of an expression or simply if we need to know if it's a constant value we can use some functions like get_expression_value() or const_expression_value() depending on the type of warning needed if the expression is not constant (for various definition of constant). However none of these functions take in account the fact that a comma expression can also have a value known at compile time. In order to not introduce unwanted change elsewheer in the code, introduce a new function expr_truth_value() which never warn but will return -1 if the expr have no known boolean value. Note: the whole set of functions should be unified to take comma expressions in account when needed. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- expand.c | 30 ++++++++++++++++++++++++++++++ expression.h | 1 + 2 files changed, 31 insertions(+)