change breakpoint syntax? about godebug

[omeid]

👍 // godebug: break

Throw an error/warning when a breakpoint is not possible.

[glycerine]

@omeld 's suggestion seems to resolve the problem of improper placement, just error out.

I particularly like this comment based breakpoint idea also because then break points could have meaningful names/phrases attached to them rather than being known by numbers only when enabling/disabling/deleting/making conditional/managing breakpoints.

e.g.

// godebug: break "top of main event loop"

Granted this isn't needed when debugging in, say, emacs; but until you get full source code for context, knowing which breakpoint you've stopped on can instantly give you the relevant context.

[jeremyschlatter]

Nice, I like the labeled breakpoint idea.

[jeremyschlatter]

I implemented something different:

_ = "breakpoint"

I prefer the look of // godebug: break, but implementing it was trickier than I expected. Since comments are not part of the AST, it's not clear where to put the breakpoint for a comment in all cases. Here's an example weird case:

for {
    fmt.Println("y")
} // godebug: break

The go/ast package provides a mapping function between AST nodes and associated comments, but there is a fair amount of extra work to do to go from an arbitrary AST statement node to the appropriate place in the tree to insert a breakpoint function call.

_ = "breakpoint, on the other hand, is straightforward. It is parsed as part of the AST. All we have to do is replace its node with the AST representation of godebug.SetTraceGen(ctx).

I considered _ = "godebug: break", but thought it looked too complicated. I expect _ = "breakpoint" to be easier to memorize.

[glycerine]

Very clever solution. I especially like that its a no-op without godebug, so I can leave it in the code. Can _ = "breakpoint" style breakpoints have comments on top? I'm imagining a conditional breakpoint syntax.

for i:=1; i < 200; i++ {
_ = "breakpoint" // if i == 100
}

[jeremyschlatter]

You can do conditional breakpoints like this for now:

for i:=1; i < 200; i++ {
    if i == 100 {
        _ = "breakpoint"
    }
}

[glycerine]

I know. I just thought it would blow people's minds when they saw it. ;-)

[jeremyschlatter]

I do like getting to blow people's minds :) I'll think about it.

[omeid]

I am not sure if I am liking the new syntax. Having to change code for break points is not the best way to go in my opinion.

I understand that comment basted instrumentation maybe slightly slow but some proper caching can mitigate that issue.

[dmitshur]

blow people's minds

As far as I know, most users of Go prefer simple, obvious, dumb solutions, rather than shocking, unexpected, complex or mind-blowing. :)

[jeremyschlatter]

I am not sure if I am liking the new syntax. Having to change code for break points is not the best way to go in my opinion.

What do you think is bad about changing the code like this for a breakpoint?

I understand that comment basted instrumentation maybe slightly slow but some proper caching can mitigate that issue.

Slowness is not the issue with using comment syntax. When I said "there is a fair amount of extra work to do to go from an arbitrary AST statement node to the appropriate place in the tree", I meant that there was a fair amount of extra thought and programming work to do, not that it would be slow at runtime.

_ = "breakpoint" was just a lot easier to implement.