Raku / nqp

NQP
Other
336 stars 131 forks source link

$brackets definition in HLL::Grammar has duplicate bracket pairs #800

Closed tbrowder closed 1 year ago

tbrowder commented 1 year ago

Variable $brackets is a string composed of bracket pairs of chars. There are some duplicated pairs.

The leading pairs are shown in their actual form inside apostrophes while the remainder are shown as a concatenated string of paired hex codepoints. When the formed chars are converted to their hex codepoints, it becomes obvious there are duplicate pairs (they match the first eight codepoints). Unless there is a reason for it, the duplicate pairs should be removed; otherwise, the reason should be documented near the definition.

The visible pairs: '<>[](){}'

Their hex codepoints: "\x[003C]\x[003E]\x[005B]\x[005D]\x[0028]\x[0029]\x[007B]\x[007D]"

lizmat commented 1 year ago

Looks like 7fed81d6f730d02eabef78960d1bec095047ea2e done my @samcv 6+ years ago.

I guess the duplicates can be removed.

lizmat commented 1 year ago

Fixed with https://github.com/Raku/nqp/commit/30593cdc28 Thanks for the spot!

MasterDuke17 commented 1 year ago

I suspect having those common delimiters at the start is a micro-optimization, since $brackets is the haystack of nqp::index calls. Haven’t measured how noticeable it is though, probably the real-world impact isn’t all that great.Sent from my iPhoneOn Feb 19, 2023, at 7:30 AM, Elizabeth Mattijsen @.***> wrote: Closed #800 as completed.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

lizmat commented 1 year ago

Well, if that's the case... the first 4 pairs are still the first 4 pairs, just in a slightly different order

MasterDuke17 commented 1 year ago

Ah nice, I also couldn’t easily check what the hex was, so we should be all good.Sent from my iPhoneOn Feb 19, 2023, at 8:25 AM, Elizabeth Mattijsen @.***> wrote: Well, if that's the case... the first 4 pairs are still the first 4 pairs, just in a slightly different order

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

tbrowder commented 1 year ago

Shall we order the pairs by the left bracket codepoint? I have already created a new table with the order as is. But it's easy to reorder if need be.

lizmat commented 1 year ago

Problem with that approach is that we either would need two tables (one for openers, one for closers), or we would always need to include the length of the array in the calculation on where to find the closer for a given opener.

So if we're refactoring this, I think having two tables would make the most sense, as the index of the opener would be the index for the closer.

Of course with appropriate documentation that these two lists should be in sync :-)

tbrowder commented 1 year ago

I probably confused the issue. I was talking about a table for the docs, not changing the HLL::Grammar. I always meant that docs table as a user reference for finding valid bracket pairs. I know it was always kind of worthless as a table of openers only, but I wasn't smart enough to find the HLL::Grammar $brackets string at the time--my primary motivation was to generate a complex table.