-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
OTP 21 compatibility when using stack traces #193
Conversation
There is no good solution if you want BOTH a) be able to compile with both pre- and post-21 compilers b) no warning (e.g. if you use warnings_as_errors) This is, hopefully, one of the less bad hacks. We introduce the macro 'try_with_stack/1' which runs it's argument inside a try. It returns either {ok, Result} or {Class, Reason, Stacktrace}. The macro has two different implementations; post-21 (try catch C:R:S) and pre-21 (get_stacktrace/0). We use the 'OTP_RELEASE' directive, which was introduced in 21, to decide between post- and pre-21.
Have you looked at https://github.com/g-andrade/stacktrace_compat I was going to do a PR to add this, but it seems there's already two PRs related to get_stacktrace's deprecation. |
Had not seen stacktrace_compat. looks nice. |
Sorry for not having looked at this sooner. I actually wanted to merge the solution from this PR and #192 because I like keeping the try-catch statements as-is. As for the stack trace library, I would like to avoid including it since I don't want to have any dependencies, especially for such a small thing, and I agree with @massemanet that parse transforms should be avoided if possible. @massemanet Do you think you could refactor this to keep the try-catch statements intact? I prefer your code because of the clarity and documentation. 🙂 |
sure. I disliked #192 because the |
@massemanet Ah, missed that nuance. I guess one alternative would be two macros? -ifdef(OTP_RELEASE).
-define(STACKTRACE, St0).
-define(WITH_STACKTRACE(T, R), T:R:St0).
-else.
-define(STACKTRACE, erlang:get_stacktrace()).
-define(WITH_STACKTRACE(T, R, S), T:R).
-endif.
% then
try
X
catch
?WITH_STACKTRACE(Class, Reason) ->
St = ?STACKTRACE,
% ...
end
|
I disliked the two macro solution too, because now you have two problems (or three?)... (Real problem might be that I'm just an internet hater...) |
I see now that the S in |
I'm OK with whatever solution, as long as it works seemlessly with OTP-19 and OTP-21, for example. |
@massemanet Looks good so far. A few more issues/questions before I merge:
|
I also ran dialyzer. There's some warnings, but none seems related to my changes (I did fix an unrelated buggy spec) |
Looks very good! Thanks! |
Any chance this gets a tag soon? |
@paulo-ferraz-oliveira Thanks for reminding me. I'll release a new version today! |
@paulo-ferraz-oliveira @massemanet Fix is in 0.8.11. |
@eproxus: thanks much; it's working as expected. |
Thanks @massemanet and @getong! |
There is no good solution if you want BOTH
a) be able to compile with both pre- and post-21 compilers
b) no warning (e.g. if you use warnings_as_errors)
This is, hopefully, one of the less bad hacks.