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

Adding test case and fix for #26 #28

Closed
wants to merge 6 commits into from
Closed

Adding test case and fix for #26 #28

wants to merge 6 commits into from

Conversation

gnois
Copy link
Contributor

@gnois gnois commented Aug 20, 2015

Added test case for gnois/luajit-lang-toolkit#2

For #26, the string.format("%q", ...) seems not able to print escapes such as \n, \r verbatim no matter how I try.
Anyhow I am not sure if the fix is the best way.

@gnois gnois changed the title Please review Fix for #26 Aug 20, 2015
@gnois gnois changed the title Fix for #26 Adding fix Aug 20, 2015
@gnois gnois changed the title Adding fix Adding test case for gnois/luajit-lang-toolkit and fix for #26 Aug 20, 2015
@gnois gnois changed the title Adding test case for gnois/luajit-lang-toolkit and fix for #26 Adding test case for gnois/luajit-lang-toolkit#2 and fix for #26 Aug 20, 2015
@gnois gnois changed the title Adding test case for gnois/luajit-lang-toolkit#2 and fix for #26 Adding test case and fix for #26 Aug 20, 2015
@franko
Copy link
Owner

franko commented Aug 20, 2015

Sorry, I was not clear. I mean:

function escape(val)
    local q = string.format("%q", val)
    return string.gsub(q, "\\\n", "\\n")
end

As for the implementation differences of "%q" I don't think that's a real issue. For me luajit's "%q" implementation is fine, except the problem with "\n" you was pointing out.
Re-implementing such a function in Lua can be very costly and should be avoided IMHO.

@gnois
Copy link
Contributor Author

gnois commented Aug 20, 2015

What about other escapes like \r \f \t \\ \' \" ?

@franko
Copy link
Owner

franko commented Aug 20, 2015

Ok, I see your point. I think it is ok to merge your proposition.

Now I'm just wondering why in your "escape" function ' and " are not in the list of all the other escape sequences. You are calling gsub for them outside of the for loop but I don't see why they are not treated like the others, in the for loop.

@franko
Copy link
Owner

franko commented Aug 20, 2015

Ok, I found something cool with just one gsub call:

local function replace(c)
    local esc = {
        ['\a'] = [[\a]], ['\b'] = [[\b]], ['\f'] = [[\f]], ['\n'] = [[\n]], ['\r'] = [[\r]], ['\t'] = [[\t]], ['\v'] = [[\v]], ['\''] = [[\']], ['\"'] = [[\"]]
    }
    return esc[c] and esc[c] or ('\\' .. string.format("%d", string.byte(c)))
end

local function escape(s)
    return string.gsub(s, "%c", replace)
end

I didn't make any measurement but I think it should be more fast to execute. With this function the string is scanned just one time and no temporary strings are created except the bit to represent a char in decimal notation.

@gnois
Copy link
Contributor Author

gnois commented Aug 21, 2015

Yeah, that sounds better! I am not aware that there's a pattern that match control chars. :)
Allow me to add some changes still, because the "%c" does not catch \\ \" which are non-controls.

local function replace(c)
    local esc = {
        ['\\'] = [[\\]], ['"'] = [[\"]], ['\a'] = [[\a]], ['\b'] = [[\b]], ['\f'] = [[\f]], ['\n'] = [[\n]], ['\r'] = [[\r]], ['\t'] = [[\t]], ['\v'] = [[\v]]
    }
    return esc[c] or string.format("\\%d", string.byte(c))
end

local function escape(s)
    -- \' is not needed since we quote with \"
    return string.gsub(s, '[%c\\"]', replace)
end

Without the change, the following would not pass.

print("\\\n\r\"'\'\0")
print('\v\\\t\"\'')

Forget about my previous implementation. The \\ is out of the loop because I think it has to be replaced earlier than the control chars, as pairs() does not observe ordering. The \' and \" are outside without good reason, other than they are punctuations like \\, which is non sense.

@franko
Copy link
Owner

franko commented Aug 21, 2015

Ok, I found a good solution and I made the change directly in the master branch. Here the commit 738f577.

@gnois
Copy link
Contributor Author

gnois commented Aug 22, 2015

Alright, thanks.

@franko franko closed this Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants