From 0554d760fb53dda6222e934720960c329e5ada10 Mon Sep 17 00:00:00 2001 From: Sasha Smundak Date: Thu, 8 Jul 2021 18:26:12 -0700 Subject: [PATCH] Allow $(filter ...) with arbitrary args if its result is compared to the empty string Bug: 172923994 Test: internal Change-Id: Iea36ecaa8940cf4e495ad63125f10d733c3eb2ee --- mk2rbc/expr.go | 26 +++++++++------- mk2rbc/mk2rbc.go | 72 ++++++++++++++++++++++++------------------- mk2rbc/mk2rbc_test.go | 25 +++++++++++++-- 3 files changed, 77 insertions(+), 46 deletions(-) diff --git a/mk2rbc/expr.go b/mk2rbc/expr.go index b06ed90e3..915f69efc 100644 --- a/mk2rbc/expr.go +++ b/mk2rbc/expr.go @@ -240,22 +240,21 @@ func (eq *eqExpr) eval(valueMap map[string]starlarkExpr) (res starlarkExpr, same } func (eq *eqExpr) emit(gctx *generationContext) { - // Are we checking that a variable is empty? - var varRef *variableRefExpr - if s, ok := maybeString(eq.left); ok && s == "" { - varRef, ok = eq.right.(*variableRefExpr) - } else if s, ok := maybeString(eq.right); ok && s == "" { - varRef, ok = eq.left.(*variableRefExpr) - } - if varRef != nil { - // Yes. + emitSimple := func(expr starlarkExpr) { if eq.isEq { gctx.write("not ") } - varRef.emit(gctx) - return + expr.emit(gctx) } + // Are we checking that a variable is empty? + if isEmptyString(eq.left) { + emitSimple(eq.right) + return + } else if isEmptyString(eq.right) { + emitSimple(eq.left) + return + } // General case eq.left.emit(gctx) if eq.isEq { @@ -578,3 +577,8 @@ func maybeConvertToStringList(expr starlarkExpr) starlarkExpr { } return expr } + +func isEmptyString(expr starlarkExpr) bool { + x, ok := expr.(*stringLiteralExpr) + return ok && x.literal == "" +} diff --git a/mk2rbc/mk2rbc.go b/mk2rbc/mk2rbc.go index 86e647d5f..ca9431f7b 100644 --- a/mk2rbc/mk2rbc.go +++ b/mk2rbc/mk2rbc.go @@ -938,14 +938,14 @@ func (ctx *parseContext) parseCheckFunctionCallResult(directive *mkparser.Direct func (ctx *parseContext) parseCompareFilterFuncResult(cond *mkparser.Directive, filterFuncCall *callExpr, xValue starlarkExpr, negate bool) starlarkExpr { // We handle: - // * ifeq/ifneq (,$(filter v1 v2 ..., $(VAR)) becomes if VAR not in/in ["v1", "v2", ...] - // * ifeq/ifneq (,$(filter $(VAR), v1 v2 ...) becomes if VAR not in/in ["v1", "v2", ...] + // * ifeq/ifneq (,$(filter v1 v2 ..., EXPR) becomes if EXPR not in/in ["v1", "v2", ...] + // * ifeq/ifneq (,$(filter EXPR, v1 v2 ...) becomes if EXPR not in/in ["v1", "v2", ...] // * ifeq/ifneq ($(VAR),$(filter $(VAR), v1 v2 ...) becomes if VAR in/not in ["v1", "v2"] // TODO(Asmundak): check the last case works for filter-out, too. xPattern := filterFuncCall.args[0] xText := filterFuncCall.args[1] var xInList *stringLiteralExpr - var xVar starlarkExpr + var expr starlarkExpr var ok bool switch x := xValue.(type) { case *stringLiteralExpr: @@ -955,34 +955,42 @@ func (ctx *parseContext) parseCompareFilterFuncResult(cond *mkparser.Directive, // Either pattern or text should be const, and the // non-const one should be varRefExpr if xInList, ok = xPattern.(*stringLiteralExpr); ok { - xVar = xText + expr = xText } else if xInList, ok = xText.(*stringLiteralExpr); ok { - xVar = xPattern + expr = xPattern + } else { + return &callExpr{ + object: nil, + name: filterFuncCall.name, + args: filterFuncCall.args, + returnType: starlarkTypeBool, + } } case *variableRefExpr: if v, ok := xPattern.(*variableRefExpr); ok { if xInList, ok = xText.(*stringLiteralExpr); ok && v.ref.name() == x.ref.name() { // ifeq/ifneq ($(VAR),$(filter $(VAR), v1 v2 ...), flip negate, // it's the opposite to what is done when comparing to empty. - xVar = xPattern + expr = xPattern negate = !negate } } } - if xVar != nil && xInList != nil { - if _, ok := xVar.(*variableRefExpr); ok { - slExpr := newStringListExpr(strings.Fields(xInList.literal)) - // Generate simpler code for the common cases: - if xVar.typ() == starlarkTypeList { - if len(slExpr.items) == 1 { - // Checking that a string belongs to list - return &inExpr{isNot: negate, list: xVar, expr: slExpr.items[0]} - } else { - // TODO(asmundak): - panic("TBD") - } + if expr != nil && xInList != nil { + slExpr := newStringListExpr(strings.Fields(xInList.literal)) + // Generate simpler code for the common cases: + if expr.typ() == starlarkTypeList { + if len(slExpr.items) == 1 { + // Checking that a string belongs to list + return &inExpr{isNot: negate, list: expr, expr: slExpr.items[0]} + } else { + // TODO(asmundak): + panic("TBD") } - return &inExpr{isNot: negate, list: newStringListExpr(strings.Fields(xInList.literal)), expr: xVar} + } else if len(slExpr.items) == 1 { + return &eqExpr{left: expr, right: slExpr.items[0], isEq: !negate} + } else { + return &inExpr{isNot: negate, list: newStringListExpr(strings.Fields(xInList.literal)), expr: expr} } } return ctx.newBadExpr(cond, "filter arguments are too complex: %s", cond.Dump()) @@ -990,7 +998,7 @@ func (ctx *parseContext) parseCompareFilterFuncResult(cond *mkparser.Directive, func (ctx *parseContext) parseCompareWildcardFuncResult(directive *mkparser.Directive, xCall *callExpr, xValue starlarkExpr, negate bool) starlarkExpr { - if x, ok := xValue.(*stringLiteralExpr); !ok || x.literal != "" { + if !isEmptyString(xValue) { return ctx.newBadExpr(directive, "wildcard result can be compared only to empty: %s", xValue) } callFunc := wildcardExistsPhony @@ -1006,19 +1014,19 @@ func (ctx *parseContext) parseCompareWildcardFuncResult(directive *mkparser.Dire func (ctx *parseContext) parseCheckFindstringFuncResult(directive *mkparser.Directive, xCall *callExpr, xValue starlarkExpr, negate bool) starlarkExpr { - if x, ok := xValue.(*stringLiteralExpr); !ok || x.literal != "" { - return ctx.newBadExpr(directive, "findstring result can be compared only to empty: %s", xValue) - } - return &eqExpr{ - left: &callExpr{ - object: xCall.args[1], - name: "find", - args: []starlarkExpr{xCall.args[0]}, - returnType: starlarkTypeInt, - }, - right: &intLiteralExpr{-1}, - isEq: !negate, + if isEmptyString(xValue) { + return &eqExpr{ + left: &callExpr{ + object: xCall.args[1], + name: "find", + args: []starlarkExpr{xCall.args[0]}, + returnType: starlarkTypeInt, + }, + right: &intLiteralExpr{-1}, + isEq: !negate, + } } + return ctx.newBadExpr(directive, "findstring result can be compared only to empty: %s", xValue) } func (ctx *parseContext) parseCompareStripFuncResult(directive *mkparser.Directive, diff --git a/mk2rbc/mk2rbc_test.go b/mk2rbc/mk2rbc_test.go index 240d0b8ca..fa3cd83f8 100644 --- a/mk2rbc/mk2rbc_test.go +++ b/mk2rbc/mk2rbc_test.go @@ -342,6 +342,8 @@ ifneq (,$(filter plaf,$(PLATFORM_LIST))) endif ifeq ($(TARGET_BUILD_VARIANT), $(filter $(TARGET_BUILD_VARIANT), userdebug eng)) endif +ifneq (,$(filter true, $(v1)$(v2))) +endif `, expected: `load("//build/make/core:product_config.rbc", "rblf") @@ -349,12 +351,14 @@ def init(g, handle): cfg = rblf.cfg(handle) if g["TARGET_BUILD_VARIANT"] not in ["userdebug", "eng"]: pass - if g["TARGET_BUILD_VARIANT"] in ["userdebug"]: + if g["TARGET_BUILD_VARIANT"] == "userdebug": pass if "plaf" in g.get("PLATFORM_LIST", []): pass if g["TARGET_BUILD_VARIANT"] in ["userdebug", "eng"]: pass + if "%s%s" % (_v1, _v2) == "true": + pass `, }, { @@ -385,8 +389,23 @@ endif def init(g, handle): cfg = rblf.cfg(handle) if g["TARGET_PRODUCT"] not in ["yukawa_gms", "yukawa_sei510_gms"]: - if g["TARGET_PRODUCT"] in ["yukawa_gms"]: + if g["TARGET_PRODUCT"] == "yukawa_gms": pass +`, + }, + { + desc: "filter $(V1), $(V2)", + mkname: "product.mk", + in: ` +ifneq (, $(filter $(PRODUCT_LIST), $(TARGET_PRODUCT))) +endif +`, + expected: `load("//build/make/core:product_config.rbc", "rblf") + +def init(g, handle): + cfg = rblf.cfg(handle) + if rblf.filter(g.get("PRODUCT_LIST", ""), g["TARGET_PRODUCT"]): + pass `, }, { @@ -779,7 +798,7 @@ endif def init(g, handle): cfg = rblf.cfg(handle) - if rblf.mkstrip(g.get("TARGET_VENDOR", "")) != "": + if rblf.mkstrip(g.get("TARGET_VENDOR", "")): pass `, },