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.

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

Add echo() and assert() #1587

Closed
wants to merge 4 commits into from
Closed

Conversation

t-paul
Copy link
Member

@t-paul t-paul commented Feb 21, 2016

Fixes #381

TODO:

  • fix toHtmlEscaped which is only available in Qt5. See Qt::escape(src)
  • Create new issue for (Documentation, Cheat sheet and example)
  • Code review (@kintel)
    • General & tests. -> Some minor cleanup possible.
    • Evaluate stack trace proposal. -> I believe this can be cleanly applied later -> separate issue
    • Evaluate message= proposal -> Consider rewriting assert() -> assert(condition=, message=) (i.e. single condition). Might need more discussion
  • Sequential assignment in echo() breaks backwards compatibility. Someone might rely on the old behavior. How to deal with that? See echotest_echo-tests, which fails with --enable=echo.
  • Merge with master (e.g. resolve the new type of "function calls", perhaps in a new supertype)

This adds assert() as expression and statement and echo() as expression. In addition the echo() statement is aligned with the new behavior of using sequential assignment and return child modules.

echo expression:

a = 3;
b = 6;
t6 = echo(c = 2, d = c + 9) a*b*c*d;
echo(t6 = t6);

// ECHO: c = 2, d = 11, 396
// ECHO: t6 = 396

assert expression:

a = 10;
b = 20;
v = assert(a < 20 && b < 20, "Test! <html>&</html>") a + b;

// ERROR: Assertion '((a < 20) && (b < 20))' failed: "Test! <html>&</html>"

a = 8;
b = 18;
v = assert(condition = a < 20 && b < 20) a + b;
echo(v);

// ECHO: 26

assert statement:

assert(false);

// ERROR: Assertion 'false' failed

a = 8;
b = 6;
assert(a < 10 && b < 8) { cube(a); sphere(b); }

// produces both cube and sphere as long as both assertion expressions are true

{
inst->scope.apply(c);
std::vector<AbstractNode *> instantiatednodes = inst->instantiateChildren(&c);
node->children.insert(node->children.end(), instantiatednodes.begin(), instantiatednodes.end());
Copy link
Member

Choose a reason for hiding this comment

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

since applyChildren() is always called on a fresh node, it's not necessary to copy, we can just do like the GroupNode: node->children = inst->instantiateChildren(&c);

This is mainly for debugging expressions or functions as the existing
echo() is only usable on statement level.
The echo() expression works like let(), including the sequential assignment
with the additional side effect of printing all the arguments and if there
is an expression following the echo(), this expression is evaluated and
printed last.
This means the assignment is handled sequentially and a new
scope is created. Children are able to use variables from the
new scope and are otherwise just passed on to the parent without
change.
@t-paul
Copy link
Member Author

t-paul commented Feb 28, 2016

Updated assert to use assert(condition, message).

@dcousens
Copy link

dcousens commented Apr 9, 2016

LGTM, will test soon

@greyltc
Copy link

greyltc commented Oct 22, 2016

Is there anything I can do to help with getting this PR accepted and merged? I'd love to have this feature.

@kintel
Copy link
Member

kintel commented Oct 22, 2016

@greyltc The sequential assignment issue needs to be resolved, which I guess needs some discussion. Apart from that, help with documentation/cheat sheet and an example would be cool.

@doug-moen
Copy link
Contributor

@kintel said "The sequential assignment issue needs to be resolved"

My view is that echo() should not have sequential assignment semantics.

  1. It's not backward compatible.
  2. It's not the right design. In general, function & module calls with labeled parameters should not introduce a new scope, they shouldn't have sequential assignment semantics.
    • It's confusing to developers who have used other programming languages with labeled parameters, since no other language works this way.
    • It gets in the way, it's a barrier to programming. There are legitimate reasons you might have local variables or parameters with the same name as labels used in a function call. It's annoying and inconvenient if those variables are shadowed and made inaccessible (and silently substituted) within a function call. I argue this case in more detail in Sequential assignment of default module arguments #1217.

@t-paul
Copy link
Member Author

t-paul commented Oct 22, 2016

It would simply behave like let(). Is that considered wrong too?

@t-paul t-paul mentioned this pull request Oct 23, 2016
@t-paul
Copy link
Member Author

t-paul commented Oct 23, 2016

As the pull request is pretty much dead due to the massive conflicts, I've extracted the assert() part to a new pull request #1827.

I'll wait with echo() until it's clarified.

@nophead
Copy link
Member

nophead commented Oct 25, 2016

Since echo in functions is for debugging, it seems wrong to me that it can
have side effects. On the other hand let is there to assign variables.

On 22 October 2016 at 23:42, Torsten Paul notifications@github.com wrote:

It would simply behave like let(). Is that considered wrong too?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1587 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAijhffHy6YsqHxo6H85Fl-NFBMYt9xEks5q2pFpgaJpZM4HfCW_
.

@kintel
Copy link
Member

kintel commented Oct 25, 2016

@nophead has a good point. Are there any obvious uses for this feature?

@doug-moen
Copy link
Contributor

What @nophead said. You should be able to comment out an echo(...) without it changing the meaning of the program, but that's not safe if echo(args)expr binds names within expr.

@MichaelAtOz
Copy link
Member

-1 let() style.
+1 get echo() expression into dev snapshot for testing

@t-paul
Copy link
Member Author

t-paul commented Oct 25, 2016

There's nothing to get in, so far it's only listing what is not wanted (actually we may want to move the discussion to some new issue as that pull request is dead regarding the code attached).

There's no need that echo() behaves exactly like let(). That comment related more to the general statement that sequential assignments are evil, yet we introduced those and there was discussion to use it in other places too. So that leaves me confused.

As for the side effect, that's certainly a point that makes sense. To get forward, maybe we should leave echo() as it is module only and have a new function/module that has a nicer interface.

@nophead
Copy link
Member

nophead commented Oct 25, 2016

Why can't echo in a function work just the same as the statement version?

People have been desperate for this for ages. Several times I have
translated OpenScad functions into Python to debug them. Like many things
in OpenScad the discussion goes round in circles and features that people
have already implemented never get included. There was an echof years ago.
Not sure why it wasn't just called echo, but it worked. If a simple debug
function can't be agreed upon I can't see anything else moving forward.

On 25 October 2016 at 22:27, Torsten Paul notifications@github.com wrote:

There's nothing to get in, so far it's only listing what is not wanted
(actually we may want to move the discussion to some new issue as that pull
request is dead regarding the code attached).

There's no need that echo() behaves exactly like let(). That comment
related more to the general statement that sequential assignments are evil,
yet we introduced those and there was discussion to use it in other places
too. So that leaves me confused.

As for the side effect, that's certainly a point that makes sense. To get
forward, maybe we should leave echo() as it is module only and have a new
function/module that has a nicer interface.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1587 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAijhUz2Gs8ahpOGGEt6BQYnkBekQZ2Vks5q3nRVgaJpZM4HfCW_
.

@t-paul
Copy link
Member Author

t-paul commented Oct 25, 2016

Using the echo() expression style, there would be a nice way to just insert the debug statement into an existing expression. Assuming "normal" style, there's no good way to give it an obvious return value, just defining a random parameter name like echo(a = "hallo", return = 3); to be the expression value seems ugly and might even cause issues for the implementation. Always returning 0 or undef also means to find a neutral solution that does not break the original expression defeating the point of easy debugging a bit.

@kintel
Copy link
Member

kintel commented Oct 25, 2016

It sounds like echo() expression without any side effects would be a good middle way for this.

As @t-paul alluded towards, the old fecho suggestion was a very, early external hack, which wasn't even close to be merged, but which eventually triggered @t-paul to build a better one himself.

@t-paul
Copy link
Member Author

t-paul commented Oct 27, 2016

See #1830 for updated echo() code, closing this one.

@t-paul t-paul closed this Oct 27, 2016
@t-paul t-paul mentioned this pull request Oct 27, 2016
@thehans
Copy link
Member

thehans commented Jan 18, 2017

Could assert be made to handle multiple expressions, comma separated?

This code passes without any error, is that intended? assert(true, false);

@krichter722
Copy link

Could assert be made to handle multiple expressions, comma separated?

This code passes without any error, is that intended? assert(true, false);

This can be handled with assert(true && false) or assert(true); assert(false); and thus isn't compatible with KISS.

Does OpenSCAD have an assert statement, now, or not? https://en.wikibooks.org/wiki/OpenSCAD_User_Manual/Other_Language_Features#assert says yes, source build of openscad-2015.03-1-882-ge990ac49e (tag update wouldn't hurt) says WARNING: Ignoring unknown module 'assert'. , i.e. no. This discussion above is slightly confuse and the number and complexity of references to pull requests difficult. echo prints text, let is a scope modifier and assert is a do-or-die compile or runtime conditional check. All three have no intersection of functionality and should be implemented as intuitive as possible.

@kintel
Copy link
Member

kintel commented Feb 13, 2017

@krichter722 It's still experimental and must be enabled from Preferences. See here for the TODO list for next step: #1860

@MichaelAtOz
Copy link
Member

Does OpenSCAD have an assert statement, now, or not?

As the wiki mentions: "[Note: Requires version 2017.01 Experimental Build]"
It is in snapshots, I'm using 2017.01.20 and it's there.

@t-paul t-paul deleted the echo-and-assert branch October 7, 2018 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

an assert function forcing compilation to quit
9 participants