SDLang-D icon indicating copy to clipboard operation
SDLang-D copied to clipboard

Improved memory safety and purity

Open ZILtoid1991 opened this issue 6 years ago • 1 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.

ZILtoid1991 avatar Aug 08 '19 20:08 ZILtoid1991

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.

Abscissa avatar Nov 21 '19 05:11 Abscissa