-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
replace / to . on fun_literal_name #6058
Conversation
I think just |
I don't know, It is used to be with gsub on 0.24.2 on previous https://github.com/crystal-lang/crystal/blob/0.24.2/src/compiler/crystal/codegen/codegen.cr#L2045 but somehow on master, |
Looks like gsub do more memory allocations but it is indeed faster, so I will update this. |
I'm very much against this change. This is a path, and as such it needs to have slashes in it. I'm actively using it in raven.cr for instance. |
OK, closing because I don't care. But this is NOT the path by the point of view on LLVM. You're just using side effect, which already not working on Windows. |
This is mangling the symbol name, not mangling the debug info. I really don't have an opinion on this patch but it won't (shouldn't) affect backtraces unless you use |
He is extracting backtrace messages with regexes, thats why he don't like. |
Well breaking someone's brittle backtrace parsing isn't a good enough reason to reject the PR. |
@RX14 perhaps not, but I'd like to see other way for extracting such information from a backtrace which doesn't provide it otherwise, like in case of |
What's not brittle about parsing backtraces with regexes? We make no guarantees about backtraces, and their syntax may well change well after 1.0. |
@RX14 well, 100% parsing reliability doesn't seem brittle at all to me. |
This is mangling specific to a Proc. I included the line because it was cute and at that time we didn't have accurate backraces (well, now we don't, either, but we are close). The mangling for Proc should be changed, and it should not include the path/line/column in it. |
In any case having accurate backtraces (including path/line/column information) I'd say is pretty important, especially for tools like sentry/raven. |
Yes, but that has nothing to do with the mangling used for a Proc. Basically, if you have: ->{ 1 + 2 } that will generate an LLVM function whose name has the location in it, which is totally useless, as that location is also recorded if you compile with debug info. |
@asterite IMO that's alright as long as that location is going to be presented within the backtrace. |
LLVM's opt tool generate .dot files on each function like following
opt hello.ll -dot-cfg > /dev/null
but on linux you will get an error with slash in filename.
This PR should fixes that to following