dlang-community / SDLang-D

An SDLang (Simple Declarative Language) library for D
http://sdlang.org
Other
120 stars 21 forks source link

Fix line counter for Windows-style line endings. Fixes #28. #29

Closed s-ludwig closed 9 years ago

s-ludwig commented 9 years ago

Not sure if using an int return is the most elegant solution, but it seems to work for all combinations.

Also fixes the "unittest" configuration in package.json and fixes the line numbers reported by testLex.

BTW, unrelated to this fix, when running "dub test" (on Windows), I'm getting an access violation in Fiber.state:

object.Error@(0): Access Violation
----------------
0x004BD669 in const(nothrow @property core.thread.Fiber.State function()) core.thread.Fiber.state
0x00488E42 in D15libInputVisitor272__T12InputVisitorTS6sdlang6parser10PullParserTS3std7variant200__T8VariantNE4542722389B3E902D4E76A676A7CBA4 at C:\Users\sludwig\Develop\SDLang-D\..\..\AppData\Roaming\dub\packages\libinputvisitor-1.1.0\libInputVisitor.d(72)
0x00488CB4 in void sdlang.parser.__unittestL527_13() at C:\Users\sludwig\Develop\SDLang-D\src\sdlang\parser.d(534)
0x00489114 in void sdlang.parser.__modtest()
0x004C9E13 in int core.runtime.runModuleUnitTests().__foreachbody1(object.Module
Info*)
Abscissa commented 9 years ago

Not sure if using an int return is the most elegant solution, but it seems to work for all combinations.

Hmm, I think if we just document isAtNewline as returning the length of the newline (in number of dchars) or 0 for "none", then that would make pretty good sense. That suggests also making it a size_t to be consistent with D convention. And maybe just for cleanliness, overload advanceChar to accept the number of chars to advance (default 1)...or not, it's not like there are any 3-code-point newlines (to my knowledge).

If you do that, and adjust the if ( to if( just to be consistent with rest of the project's style (I think I sometimes forget to do the opposite in my dub PRs, sorry bout that) then I'll merge this. Everything else looks good, thanks.

Also fixes the "unittest" configuration in package.json

Does the version identifiers changing from SDLang_Unittest/SDLang_Trace to sdlangUnittest/sdlangTrace relate to this? Not that I'm objecting to it, but I was wondering about the reason behind that change.

BTW, unrelated to this fix, when running "dub test" (on Windows), I'm getting an access violation in Fiber.state:

Ouch. I'll take a look. Can you open a separate ticket for this?

s-ludwig commented 9 years ago

Okay, I've update the PR with your suggestions. Sorry about the mismatched code style, I somehow got so used to it (after adjusting my own preferences a while ago), because virtually all D code looks like that nowadays, that I didn't even think that it could be different ;)

Does the version identifiers changing from SDLang_Unittest/SDLang_Trace to sdlangUnittest/sdlangTrace relate to this? Not that I'm objecting to it, but I was wondering about the reason behind that change.

Yes, that's the fix, because the lower-case versions are the ones that are used in the D code. Looks like they are a relict of earlier versions?

BTW, unrelated to this fix, when running "dub test" (on Windows), I'm getting an access violation in Fiber.state:

Ouch. I'll take a look. Can you open a separate ticket for this?

Seems to be #16 - it crashes in the regression test for that issue. But it looks like it was simply never fixed in the first place, commit cc626fd3b83a07fefe7418fcf384fce67671b412 just adds the regression test. And the PullParser is still stored as a pointer in InputVisitor.

s-ludwig commented 9 years ago

Oh, wait, I've just noticed that I didn't actually set the test_locations parameter for testLex. The strange thing is that the test now hangs for me after printing "Unittesting sdlang ast...", before even reaching the lexer tests - I'll investigate.

s-ludwig commented 9 years ago

Okay, fixed and rebased. Should be good to go now (old \r Mac style line endings still weren't handled correctly).

Abscissa commented 9 years ago

Does the version identifiers changing from SDLang_Unittest/SDLang_Trace to sdlangUnittest/sdlangTrace relate to this? Not that I'm objecting to it, but I was wondering about the reason behind that change.

Yes, that's the fix, because the lower-case versions are the ones that are used in the D code. Looks like they are a relict of earlier versions?

Oops, yes, that would be it.

Seems to be #16 - it crashes in the regression test for that issue. But it looks like it was simply never fixed in the first place, commit cc626fd just adds the regression test. And the PullParser is still stored as a pointer in InputVisitor.

The actual fix was in libInputVisitor, version 1.2. SDLang itself didn't need to be updated, it just needs to use the latest libInputVisitor...which...I seem to have forgotten to update package.json to require...(/facepalm)... I guess I should probably update package.json to dub.json while I'm at it. dub.json has been supported for quite a while know.