Skip to content
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

Changes needed to support line directives #919

Merged
merged 3 commits into from Nov 16, 2016

Conversation

MasterDuke17
Copy link
Contributor

An extra blank line after a directive throws off the numbering.

The m-CORE.setting file is no longer seen in a backtrace, so we need
another way of determining if we are in the settings. This isn't a
great solution, but should work for now.

Requires Raku/nqp#319

With the NQP PR, passes make spectest

An extra blank line after a directive throws off the numbering.

The m-CORE.setting file is no longer seen in a backtrace, so we need
another way of determining if we are in the settings. This isn't a
great solution, but should work for now.
@zoffixznet
Copy link
Contributor

zoffixznet commented Nov 15, 2016

IMO just checking the path for .contains("src/core") is too generic. Any project that follows Rakudo's directory structure would fail to report error message backtraces. And any other combinations are also matched: src/core-utils/my-scripts/foo.p6, /home/tombrozresrc/corespondents/some-other/code/perl6.p6, etc.

I ran this command on my box and it got 681 matches, only 594 of which are in my ~/

zoffix@VirtualBox:~$ tree -f | grep 'src/core' | grep -vi 'rakudo' | grep -vi 'judo' | wc -l
594

zoffix@VirtualBox:/$ tree -f | grep 'src/core' | grep -vi 'rakudo' | grep -vi 'judo' | wc -l
681

Looking at changes in https://github.com/perl6/nqp/pull/319/files... can't we just add some sort of special prefix to core files and filter them out that way instead? As in, if it were CORE.setting and now it's src/core/whaver, sounds like we can use arbitrary data in there...

@MasterDuke17
Copy link
Contributor Author

FWIW, some discussion with @niner and @lizmat about .contains("src/core") here.

@zoffixznet
Copy link
Contributor

This one?

<lizmat> contains('src/core') ?
<lizmat> MasterDuke: I think ^^ will be sufficient for now
<nine> So people should not develop their modules in a src/core directory?
<MasterDuke> nine: FWIW, there is already an error message somewhere if you try to compile a file ending in '.setting'

TBH, I'm not a fan of "for now" solutions. We're not in a rush; why not do it right the first time around?

In the NQP PR this line just finds the filename that the gen-cat script makes, right?:

+    regex comment:sym<line_directive> {
 +        ^^ '#' \s* 'line' \s+ $<line>=(\d+) [ \s+ $<filename>=(\S+) ]? $$
 +    }
 +

So what if we change the gen-cat script line 23 from say("#line 1 $file\n"); to something like `say("#line 1 Rakudo::Internals/$file\n");?

Then instead of src/core to detect a core file, we can use .starts-with('Rakudo::Internals/').

Is there a problem with this approach?

@MasterDuke17
Copy link
Contributor Author

I don't really have a problem with that approach. However, you'd need to remove the 'Rakudo::Internals/' when printing the backtrace to reflect the actual file paths. @jnthn didn't like modifying the values in Backtrace.pm (which my first attempt at implementing this did), but maybe simply removing a static string would be fine.

@zoffixznet
Copy link
Contributor

zoffixznet commented Nov 16, 2016

I would be fine with leaving Rakudo::Internals/ in when printing...

@lizmat
Copy link
Contributor

lizmat commented Nov 16, 2016

I'm n favour of adding a marker, but would be against using Rakudo::Internals, to prevent confusion with the Rakudo/Internals.pm file.

How about just prefixing it with SETTING:: ? That would be sort of appropriate anyway :)

and leave it in printing

@zoffixznet
Copy link
Contributor

How about just prefixing it with SETTING:: ? That would be sort of appropriate anyway :)

Sounds good to me.

@MasterDuke17
Copy link
Contributor Author

Change made, @lizmat++, @zoffixznet++

@zoffixznet zoffixznet merged commit f03ef55 into rakudo:nom Nov 16, 2016
@zoffixznet
Copy link
Contributor

Thanks a lot.

@MasterDuke17 MasterDuke17 deleted the source_file_line_numbers_v2 branch November 16, 2016 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants