Closed ZILtoid1991 closed 1 month ago
First of all, thanks for undertaking this. It’s much needed and it’s no small task. (Also, apologies for the delay.)
A few things I noticed on initial look through:
Changes unrelated to the main purpose of the PR should really be left out and, if important, submitted as separate PRs. This includes changing dub.selections.json, modifying formatting style, removing the blank unittesting main(), and adding doc comments (although I like most of the comments you added, so I'll let them through this time with the caveat below.) So those things will need to be fixed/reverted.
Please stick to the code styles already in-use by the project. In particular, the style this project uses for shorter multi-line doc comments is to prefix each line with ///
(note also the trailing space). Longer doc comments use the /++ +/
-style comment, and do NOT prefix individual lines with extra indentation or anything like *
. Non-nesting /* */
-style comments are not used. And a very minor nit, but the ditto lines use ///ditto
(all-lowercase and no space).
This is the really important one: Larger and/or higher-level functions (such as lexFile
, lexSource
, Lexer.this
, parseFile
, parseSource
, Attribute.remove
, toSDLDocument
, just to name a few examples), and really most, if not all, functions in fact, should NOT be @trusted
. This is widely regarded as a direct abuse of D's @safe
-ty system because it completely subverts all of its benefits. An @trusted
must only ever be used for simple low-level functions, and every usage of @trusted
must still be fully proven as meeting all of @safe
's normal guarantees even if the compiler itself cannot accept it as @safe
. The rule of thumb is: It's far better to make whatever little pieces of the project we currently can be @safe
(as progress towards an eventual goal of overall @safe
-ty), than to use @trusted
. The things that cannot be made @safe
without using @trusted
must then be examined on a case-by-case basis and redesigned so that they can be directly made @safe
. TBH, I would say that any instance of adding a @trusted
should be an individual PR all its own, just because of the extreme scrutiny necessary to properly validate each and every usage of @trusted
.
Watch out for stray trailing spaces and duplicated newlines. I noticed a few of these in the diff.
I noticed a few additional issues I will follow up with as line comments.
I could only perform the internal unittest (unit-threaded couldn't be used due to a function wasn't exported properly), and it has issues with certain floating-point numbers in the lexer's unittest. I'll look into that in the future, as well as adding the feature of getting tags by path.
Also note that thanks to some dependencies being left behind by the D community, I couldn't put
pure
everywhere it was possible, and instead of@safe
I had to rely on@trusted
.