New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Jul 28, 2015

Conversation

Projects
None yet
2 participants
@s-ludwig
Contributor

s-ludwig commented Jul 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Abscissa

Abscissa Jul 27, 2015

Owner

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?

Owner

Abscissa commented Jul 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 27, 2015

Contributor

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 cc626fd just adds the regression test. And the PullParser is still stored as a pointer in InputVisitor.

Contributor

s-ludwig commented Jul 27, 2015

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 cc626fd just adds the regression test. And the PullParser is still stored as a pointer in InputVisitor.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 27, 2015

Contributor

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.

Contributor

s-ludwig commented Jul 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 27, 2015

Contributor

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

Contributor

s-ludwig commented Jul 27, 2015

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

@Abscissa

This comment has been minimized.

Show comment
Hide comment
@Abscissa

Abscissa Jul 28, 2015

Owner

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.

Owner

Abscissa commented Jul 28, 2015

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.

Abscissa added a commit that referenced this pull request Jul 28, 2015

Merge pull request #29 from s-ludwig/master
Fix line counter for Windows-style line endings. Fixes #28.

@Abscissa Abscissa merged commit 1052f4a into Abscissa:master Jul 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment