From 71514c07d2dae1dfb1f8a88ff42d229288ddfc81 Mon Sep 17 00:00:00 2001 From: Cole Faust Date: Thu, 27 Jan 2022 17:21:41 -0800 Subject: [PATCH] Remove variableDefinedExpr VariableDefinedExpr was under-developed, and would not take into account if a variable was from the globals or product config dictionary. It also always emitted `g.get("VARIABLE") != None`, which is not correct behavior. In this example makefile: ``` MY_VAR := ifdef MY_VAR $(info MY_VAR is defined) else $(info MY_VAR is not defined) endif ifdef MY_UNDEFINED_VAR $(info MY_UNDEFINED_VAR is defined) else $(info MY_UNDEFINED_VAR is not defined) endif MY_VAR ?= true MY_UNDEFINED_VAR ?= true $(info MY_VAR after ?= is $(MY_VAR)) $(info MY_UNDEFINED_VAR after ?= is $(MY_UNDEFINED_VAR)) .PHONY: all all: @: ``` We get the output: MY_VAR is not defined MY_UNDEFINED_VAR is not defined MY_VAR after ?= is MY_UNDEFINED_VAR after ?= is true So we can see that even if a variable was set, it's considered not defined if it was set to an empty value. However, ?= works differently, and does require the != None, so that was left as is. Just use a variableRefExpr and rely on the fact that variables will be truthy if they're defined. Fixes: 216700361 Test: go test Change-Id: If8944da2579e8658e3fc4f666b1f3b2815f8c8b1 --- mk2rbc/expr.go | 26 -------------------------- mk2rbc/mk2rbc.go | 15 +++++---------- mk2rbc/mk2rbc_test.go | 20 ++++++++++---------- 3 files changed, 15 insertions(+), 46 deletions(-) diff --git a/mk2rbc/expr.go b/mk2rbc/expr.go index 3f355ac84..07f7ca189 100644 --- a/mk2rbc/expr.go +++ b/mk2rbc/expr.go @@ -378,32 +378,6 @@ func (eq *eqExpr) transform(transformer func(expr starlarkExpr) starlarkExpr) st } } -// variableDefinedExpr corresponds to Make's ifdef VAR -type variableDefinedExpr struct { - v variable -} - -func (v *variableDefinedExpr) emit(gctx *generationContext) { - if v.v != nil { - v.v.emitDefined(gctx) - return - } - gctx.writef("%s(%q)", cfnWarning, "TODO(VAR)") -} - -func (_ *variableDefinedExpr) typ() starlarkType { - return starlarkTypeBool -} - -func (v *variableDefinedExpr) emitListVarCopy(gctx *generationContext) { - v.emit(gctx) -} - -func (v *variableDefinedExpr) transform(transformer func(expr starlarkExpr) starlarkExpr) starlarkExpr { - // TODO: VariableDefinedExpr isn't really an expression? - return v -} - type listExpr struct { items []starlarkExpr } diff --git a/mk2rbc/mk2rbc.go b/mk2rbc/mk2rbc.go index e317cadff..17d3c2185 100644 --- a/mk2rbc/mk2rbc.go +++ b/mk2rbc/mk2rbc.go @@ -1003,19 +1003,14 @@ func (ctx *parseContext) processBranch(check *mkparser.Directive) { ctx.popReceiver() } -func (ctx *parseContext) newIfDefinedNode(check *mkparser.Directive) (starlarkExpr, bool) { - if !check.Args.Const() { - return ctx.newBadExpr(check, "ifdef variable ref too complex: %s", check.Args.Dump()), false - } - v := ctx.addVariable(check.Args.Strings[0]) - return &variableDefinedExpr{v}, true -} - func (ctx *parseContext) parseCondition(check *mkparser.Directive) starlarkNode { switch check.Name { case "ifdef", "ifndef", "elifdef", "elifndef": - v, ok := ctx.newIfDefinedNode(check) - if ok && strings.HasSuffix(check.Name, "ndef") { + if !check.Args.Const() { + return &exprNode{expr: ctx.newBadExpr(check, "ifdef variable ref too complex: %s", check.Args.Dump())} + } + v := NewVariableRefExpr(ctx.addVariable(check.Args.Strings[0]), false) + if strings.HasSuffix(check.Name, "ndef") { v = ¬Expr{v} } return &ifNode{ diff --git a/mk2rbc/mk2rbc_test.go b/mk2rbc/mk2rbc_test.go index d62882d45..b1a4b6587 100644 --- a/mk2rbc/mk2rbc_test.go +++ b/mk2rbc/mk2rbc_test.go @@ -131,7 +131,7 @@ load(":part1.star|init", _part1_init = "init") def init(g, handle): cfg = rblf.cfg(handle) rblf.inherit(handle, "part", _part_init) - if g.get("PRODUCT_NAME") != None: + if cfg.get("PRODUCT_NAME", ""): if not _part1_init: rblf.mkerror("product.mk", "Cannot find %s" % (":part1.star")) rblf.inherit(handle, "part1", _part1_init) @@ -174,7 +174,7 @@ load(":part1.star|init", _part1_init = "init") def init(g, handle): cfg = rblf.cfg(handle) _part_init(g, handle) - if g.get("PRODUCT_NAME") != None: + if cfg.get("PRODUCT_NAME", ""): if not _part1_init: rblf.mkerror("product.mk", "Cannot find %s" % (":part1.star")) _part1_init(g, handle) @@ -231,7 +231,7 @@ endif def init(g, handle): cfg = rblf.cfg(handle) - if g.get("PRODUCT_NAME") != None: + if cfg.get("PRODUCT_NAME", ""): cfg["PRODUCT_NAME"] = "gizmo" else: pass @@ -275,7 +275,7 @@ endif def init(g, handle): cfg = rblf.cfg(handle) - if g.get("PRODUCT_NAME") != None: + if cfg.get("PRODUCT_NAME", ""): # Comment pass else: @@ -296,7 +296,7 @@ endif def init(g, handle): cfg = rblf.cfg(handle) - if not g.get("PRODUCT_NAME") != None: + if not cfg.get("PRODUCT_NAME", ""): cfg["PRODUCT_NAME"] = "gizmo1" else: cfg["PRODUCT_NAME"] = "gizmo2" @@ -315,9 +315,9 @@ endif def init(g, handle): cfg = rblf.cfg(handle) - if g.get("PRODUCT_NAME") != None: + if cfg.get("PRODUCT_NAME", ""): cfg["PRODUCT_NAME"] = "gizmo" - elif not g.get("PRODUCT_PACKAGES") != None: + elif not cfg.get("PRODUCT_PACKAGES", []): # Comment pass `, @@ -509,11 +509,11 @@ endif def init(g, handle): cfg = rblf.cfg(handle) - if g.get("PRODUCT_NAME") != None: + if cfg.get("PRODUCT_NAME", ""): cfg["PRODUCT_PACKAGES"] = ["pack-if0"] - if g.get("PRODUCT_MODEL") != None: + if cfg.get("PRODUCT_MODEL", ""): cfg["PRODUCT_PACKAGES"] = ["pack-if-if"] - elif g.get("PRODUCT_NAME") != None: + elif cfg.get("PRODUCT_NAME", ""): cfg["PRODUCT_PACKAGES"] = ["pack-if-elif"] else: cfg["PRODUCT_PACKAGES"] = ["pack-if-else"]