From 097a362c8018359e1a33d0493d4aff89efe5539a Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Fri, 6 Mar 2020 09:39:30 +0100 Subject: [PATCH] Expr: check_and_promote_expr() returns IntrusivePtr Instead of returning a pseudo-boolean integer, it now returns a referenced object or nullptr on error. The old API was very error prone because of its obscure reference counting semantics. --- src/Attr.cc | 19 +++++++-------- src/Expr.cc | 69 ++++++++++++++++++++++++++++------------------------- src/Expr.h | 8 ++++++- src/Stmt.cc | 6 ++--- 4 files changed, 56 insertions(+), 46 deletions(-) diff --git a/src/Attr.cc b/src/Attr.cc index 5e8fb4bd7b..f12e227f12 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -315,10 +315,10 @@ void Attributes::CheckAttr(Attr* a) // Ok. break; - Expr* e = a->AttrExpr(); - if ( check_and_promote_expr(e, type) ) + auto e = check_and_promote_expr(a->AttrExpr(), type); + if ( e ) { - a->SetAttrExpr(e); + a->SetAttrExpr(e.release()); // Ok. break; } @@ -354,10 +354,10 @@ void Attributes::CheckAttr(Attr* a) // Ok. break; - Expr* e = a->AttrExpr(); - if ( check_and_promote_expr(e, ytype) ) + auto e = check_and_promote_expr(a->AttrExpr(), ytype); + if ( e ) { - a->SetAttrExpr(e); + a->SetAttrExpr(e.release()); // Ok. break; } @@ -379,11 +379,10 @@ void Attributes::CheckAttr(Attr* a) if ( (atype->Tag() == TYPE_TABLE && atype->AsTableType()->IsUnspecifiedTable()) ) { - Expr* e = a->AttrExpr(); - - if ( check_and_promote_expr(e, type) ) + auto e = check_and_promote_expr(a->AttrExpr(), type); + if ( e ) { - a->SetAttrExpr(e); + a->SetAttrExpr(e.release()); break; } } diff --git a/src/Expr.cc b/src/Expr.cc index c2461cddfd..64622474bc 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -3148,10 +3148,14 @@ TableConstructorExpr::TableConstructorExpr(IntrusivePtr constructor_li { Expr* idx = idx_exprs[j]; - if ( check_and_promote_expr(idx, (*indices)[j]) ) + auto promoted_idx = check_and_promote_expr(idx, (*indices)[j]); + if ( promoted_idx ) { - if ( idx != idx_exprs[j] ) - idx_exprs.replace(j, idx); + if ( promoted_idx.get() != idx ) + { + Unref(idx); + idx_exprs.replace(j, promoted_idx.release()); + } continue; } @@ -4671,7 +4675,10 @@ IntrusivePtr ListExpr::InitVal(const BroType* t, IntrusivePtr aggr) co loop_over_list(exprs, i) { Expr* e = exprs[i]; - check_and_promote_expr(e, vec->Type()->AsVectorType()->YieldType()); + auto promoted_e = check_and_promote_expr(e, vec->Type()->AsVectorType()->YieldType()); + if (promoted_e) + e = promoted_e.get(); + auto v = e->Eval(nullptr); if ( ! vec->Assign(i, std::move(v)) ) @@ -4970,35 +4977,34 @@ IntrusivePtr get_assign_expr(IntrusivePtr op1, is_init); } -int check_and_promote_expr(Expr*& e, BroType* t) +IntrusivePtr check_and_promote_expr(Expr* const e, BroType* t) { BroType* et = e->Type(); TypeTag e_tag = et->Tag(); TypeTag t_tag = t->Tag(); if ( t->Tag() == TYPE_ANY ) - return 1; + return {NewRef{}, e}; if ( EitherArithmetic(t_tag, e_tag) ) { if ( e_tag == t_tag ) - return 1; + return {NewRef{}, e}; if ( ! BothArithmetic(t_tag, e_tag) ) { t->Error("arithmetic mixed with non-arithmetic", e); - return 0; + return nullptr; } TypeTag mt = max_type(t_tag, e_tag); if ( mt != t_tag ) { t->Error("over-promotion of arithmetic value", e); - return 0; + return nullptr; } - e = new ArithCoerceExpr({AdoptRef{}, e}, t_tag); - return 1; + return make_intrusive(IntrusivePtr{NewRef{}, e}, t_tag); } if ( t->Tag() == TYPE_RECORD && et->Tag() == TYPE_RECORD ) @@ -5016,15 +5022,12 @@ int check_and_promote_expr(Expr*& e, BroType* t) if ( same_attrs(td1->attrs.get(), td2->attrs.get()) ) // Everything matches perfectly. - return 1; + return {NewRef{}, e}; } } if ( record_promotion_compatible(t_r, et_r) ) - { - e = new RecordCoerceExpr({AdoptRef{}, e}, {NewRef{}, t_r}); - return 1; - } + return make_intrusive(IntrusivePtr{NewRef{}, e}, IntrusivePtr{NewRef{}, t_r}); t->Error("incompatible record types", e); return 0; @@ -5035,23 +5038,17 @@ int check_and_promote_expr(Expr*& e, BroType* t) { if ( t->Tag() == TYPE_TABLE && et->Tag() == TYPE_TABLE && et->AsTableType()->IsUnspecifiedTable() ) - { - e = new TableCoerceExpr({AdoptRef{}, e}, {NewRef{}, t->AsTableType()}); - return 1; - } + return make_intrusive(IntrusivePtr{NewRef{}, e}, IntrusivePtr{NewRef{}, t->AsTableType()}); if ( t->Tag() == TYPE_VECTOR && et->Tag() == TYPE_VECTOR && et->AsVectorType()->IsUnspecifiedVector() ) - { - e = new VectorCoerceExpr({AdoptRef{}, e}, {NewRef{}, t->AsVectorType()}); - return 1; - } + return make_intrusive(IntrusivePtr{NewRef{}, e}, IntrusivePtr{NewRef{}, t->AsVectorType()}); t->Error("type clash", e); - return 0; + return nullptr; } - return 1; + return {NewRef{}, e}; } int check_and_promote_exprs(ListExpr* const elements, TypeList* types) @@ -5071,14 +5068,18 @@ int check_and_promote_exprs(ListExpr* const elements, TypeList* types) loop_over_list(el, i) { Expr* e = el[i]; - if ( ! check_and_promote_expr(e, (*tl)[i]) ) + auto promoted_e = check_and_promote_expr(e, (*tl)[i]); + if ( ! promoted_e ) { e->Error("type mismatch", (*tl)[i]); return 0; } - if ( e != el[i] ) - el.replace(i, e); + if ( promoted_e.get() != e ) + { + Unref(e); + el.replace(i, promoted_e.release()); + } } return 1; @@ -5138,14 +5139,18 @@ int check_and_promote_exprs_to_type(ListExpr* const elements, BroType* type) loop_over_list(el, i) { Expr* e = el[i]; - if ( ! check_and_promote_expr(e, type) ) + auto promoted_e = check_and_promote_expr(e, type); + if ( ! promoted_e ) { e->Error("type mismatch", type); return 0; } - if ( e != el[i] ) - el.replace(i, e); + if ( promoted_e.get() != e ) + { + Unref(e); + el.replace(i, promoted_e.release()); + } } return 1; diff --git a/src/Expr.h b/src/Expr.h index 5664133289..95c1167abe 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -925,7 +925,13 @@ IntrusivePtr get_assign_expr(IntrusivePtr op1, // types or a single type. // // Note, the type is not "const" because it can be ref'd. -extern int check_and_promote_expr(Expr*& e, BroType* t); + +/** + * Returns nullptr if the expression cannot match or a promoted + * expression. + */ +extern IntrusivePtr check_and_promote_expr(Expr* e, BroType* t); + extern int check_and_promote_exprs(ListExpr* elements, TypeList* types); extern int check_and_promote_args(ListExpr* args, RecordType* types); extern int check_and_promote_exprs_to_type(ListExpr* elements, BroType* type); diff --git a/src/Stmt.cc b/src/Stmt.cc index 5628535edc..c937c4c2ee 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -1450,9 +1450,9 @@ ReturnStmt::ReturnStmt(IntrusivePtr arg_e) else { - Expr* e_ = e.release(); - (void) check_and_promote_expr(e_, yt); - e = {AdoptRef{}, e_}; + auto promoted_e = check_and_promote_expr(e.get(), yt); + if ( promoted_e ) + e = std::move(promoted_e); } }