c-blake
commented
12 months ago Just as a quick acknowledgement, I am looking at this. It is a lot to consider/go through. I'm glad you like it, though and very much appreciate all this work! For personal reasons, it may be another 24 hours or more before I can really decide how to proceed/offer better feedback/etc.
2 yields is definitely better than 4. If you look at the VC history you will see I originally always just returned a seq..So, those yields were adds. I do worry a little about over-tuning micro-optimizations that may change next version or 3 of gcc/clang, and of course real time and instruction complexity are only vaguely related. So, we may need some benchmark on >1 CPUs to be more confident. Anyway, I'm still digesting it all, but wanted to post an initial reply.
SirNickolas
tmplParseis a brilliant: its syntax is so flexible yet can be parsed with a fewmemchrcalls and an ident-skipping loop. Unfortunately, current implementation is suboptimal and even buggy. I tried to fix/improve it:${(id)arg}syntax did not work at all becausea1was never assigned in the corresponding branch. If you tried to accessfmt[arg], you got aDefect. Fixed.${(}a)b}was parsed as a call with id=}a, arg=garbage, call=${(}, followed by texta)b}. That behaviour feels weird to me. I think these three options are more sensible:}a, arg=b, call=${(}a)b}. I.e., the first}is not treated as a closer for the whole call.(, arg=empty, call=${(}; followed by texta)b}. I.e., the first}does close the call, and when the parser fails to find)inside braces, it treats the whole braced fragment as an identifier.${(}a)b}. I.e., when the parser sees this gibberish, it gives up, emits it as plain text, and moves on to the next$.The first option goes against
fmtwriters must pick non-colliding braces rule. Given that it is enforced everywhere else, I doubt it makes much sense to loosen it here. Furthermore, a simple mistake (missing a)) can render the rest of thefmtinvalid, with all macro calls it might contain.Other options have none of the abovementioned flaws. I chose the third since it felt more natural to me; also, this is what current implementation does when there are no more
)characters in the string (as opposed to the case when there is one but beyond braces). Please tell if you’d like to see option 2 implemented.${(a} $bwas resulting in only text chunks. Now it recognizes the$bmacro call.${()}produced a macro call with an empty id, while${}was parsed as text. (That’s becausei0 == c1in line 437 is always false.) Fixed.seq[string]). Not having to check forarg.len != 0is more convenient.tmplParseis an inline iterator so loop body passed to it gets inserted at everyyieldstatement. That causes code bloating. Current implementation has 4yields; I restructured the code so that there are only 2 of them. (Reducing to 1 is not necessary; I’ll explain in a moment.)As a side effect, it slightly changed the way meta char’s self-escaping is handled: previously
$was emitted on its own, and now it becomes part of the following text piece (if there is one). I actually think that auto-merging (at least some) text chunks is an improvement: it leaves less work for the renderer.Current implementation yields either
(0..0, c1..<c0, 0..0)or(i0..<i1, a0..<a1, c0..<c1), and renderers are supposed to check forid.a == 0to see whether they are given a literal text chunk or a macro call. In the former case it is obvious that(0..0).a == 0— the compiler is able to do constant propagation and leave only one branch. However, in the latter it fails to prove thati0 != 0in general. Therefore, it emits a runtime check and is unable to eliminate dead code in the other branch (therefore, more code bloating).This optimization is performed by the backend compiler. I tried communicating
i0 != 0viaassert, but it is dropped in the Nim land and the backend compiler does not even see those assertions. While it is possible to employ__builtin_unreachable()/__assume(cond)for GCC/MSVC, things get harder for the JS backend. So I went with a different approach.I introduced
iterator tmplParse2*(its name is debatable), which yields 4-element tuples:(lit, id, arg, call).litis aboolthat istruefor text chunks andfalsefor macro calls. Now inserting the correct branch in the correctyieldposition is trivial for the compiler.I was considering making an
enumbut thought it isn’t better at anything. Both changingboolintoenumand adding a newenummember are breaking changes (the latter — becausecasestatements in user code will become non-exhaustive).I’ve been writing this with a disassembler in my other hand (GCC & Clang) so tiny replacements, such as
< 0vs== -1, are no accident. (< 0compiles down totest&jsor sometimes evensub&jswhile== -1is unable to offer anything better thancmp&je.)In the doc comment, definition lists are (mis)used to indent list items. I did not manage to find a way of making a definition span multiple lines so I had to keep a line longer than 80 characters.
For some reason,
nim doc, when generating HTML anchors, ignores proc parameters without an explicit type annotation when they have a default value of aconst. Probably a bug in the docgen. That affectstmplParseandtmplParse2because of theirids=alphaNumparameter. I put#tmplParse2.i,openArray[char],charanchor in the doc (incorrect but currently working). Another option is to explicitly declareidsto beset[char]— the anchor will be#tmplParse2.i,openArray[char],char,set[char], as expected.Will be glad to hear your opinion on the proposed changes.