Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Construct: use lambda this: for ternary expressions #246

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Construct: use lambda this: for ternary expressions #246

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions shared/src/main/scala/io/kaitai/struct/translators/ConstructTranslator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import io.kaitai.struct.format.Identifier
import io.kaitai.struct.ConstructClassCompiler

class ConstructTranslator(provider: TypeProvider, importList: ImportList) extends PythonTranslator(provider, importList) {
override def doIfExp(condition: Ast.expr, ifTrue: Ast.expr, ifFalse: Ast.expr): String =
s"(lambda this: ${translate(ifTrue)} if ${translate(condition)} else ${translate(ifFalse)})"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this is fundamentally wrong - it only works if the ternary is the top level operation in the expression (which is a nonsensical assumption, because it certainly does not hold in practice, maybe just coincidentally). Even something as simple as

   - id: data
     type: str
-    size: 'len > 0 ? len + 1 : 0'
+    size: '(len > 0 ? len + 1 : 0) + 1'
     encoding: ASCII

or nested ternaries (please leave aside that len was unsigned and can't be negative):

   - id: data
     type: str
-    size: 'len > 0 ? len + 1 : 0'
+    size: 'len > 0 ? len + 1 : (len < 0 ? 1 : 0)'
     encoding: ASCII

will not work. So I don't see much point in accepting this PR as it stands.

Besides, as you describe the issue, it is not specific to the ternary operation, is it? It seems likely to me that there are other expression features that require the same type of handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fair points. As far as I can tell, this is an issue anywhere this is used in combination with logic when building a Construct struct. I'm not sure what other things would trigger this behavior other than ternaries. I do see how this would break with nested ternaries though, so I'll figure out how to avoid that scenario.


override def doLocalName(s: String) = {
s match {
case Identifier.ITERATOR => "obj_"
Expand Down