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

[error messages] fix #8794 #9270 #9767 #9768 #9766

Merged
merged 11 commits into from Jan 8, 2019

Conversation

Projects
None yet
2 participants
@timotheecour
Copy link
Contributor

commented Nov 20, 2018

  • fixes #8794 => see tests/errmsgs/t8794.nim

  • fixes #9270

  • fixes #9767

  • fixes #9768 => see tests/errmsgs/t9768.nim

  • refactors some col+1 code to col + ColOffset (self documents code)

  • fixes off by 1 error in toFileLineCol which didn't add 1 to col when displaying some file(line,col) messages EDIT: was fixed in #10138

  • make getProcHeader show declared info location also for types and all routine kinds (including macros,templates) instead of just (rather arbitrarily) for iterator,proc,func,method

  • adds --forcefullpaths which is meant to force full paths everywhere that's being displayed , including toFilename which was used (indirectly) in #9270 ; that's the simplest way to fix this; and it makes sense to have a simple way to force using full paths everywhere in compiler messages
    EDIT: after feedback from @Araq in IRC, I instead extended meaning of existing listFullPaths flag, instead of adding a new flag forceFullPaths

  • fix typo system/except.nim => lib/system/excpt.nim

  • remove substr(foo, 0) hack in compiler/vm.nim which seems old and not applicable anymore:

this 'substr' prevents a strange corruption. XXX This needs to be investigated eventually but first attempts to fix it broke everything see the araq-wip-fixed-writebarrier branch.

@timotheecour timotheecour changed the title lots of fixes lots of fixes around stacktraces and error messages Nov 20, 2018

@timotheecour timotheecour force-pushed the timotheecour:pr_lots_fixes branch from adc2b2d to 4c64f74 Nov 22, 2018

@timotheecour timotheecour force-pushed the timotheecour:pr_lots_fixes branch from 4c64f74 to 102bc49 Dec 9, 2018

@timotheecour timotheecour force-pushed the timotheecour:pr_lots_fixes branch 2 times, most recently from 4182eed to 17e21be Dec 19, 2018

@timotheecour timotheecour requested a review from Araq Dec 20, 2018

@timotheecour timotheecour force-pushed the timotheecour:pr_lots_fixes branch from 17e21be to b8be25a Dec 21, 2018

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

/cc @Araq PTAL
(after your feedback on IRC, I extended meaning of existing listFullPaths flag instead of adding a new forceFullPaths flag)

Show resolved Hide resolved tests/errmsgs/treportunused.nim Outdated
@krux02

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Where are the test cases for the issues that you fixed?

@krux02

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Didn't read through all the changes (too long), all I see is a decrease of the error message quality (seen in the diff of the test case). I don't like that.

@timotheecour timotheecour reopened this Dec 24, 2018

@timotheecour timotheecour force-pushed the timotheecour:pr_lots_fixes branch from b8be25a to d163e08 Dec 24, 2018

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2018

Where are the test cases for the issues that you fixed?

see tests/errmsgs/t8794.nim for #8794

it requires #10091 to be merged first

Will follow up with more tests, but would really like #10091 to be merged first (it reuses the logic @krux02 introduced recently to failing tests)

@timotheecour timotheecour force-pushed the timotheecour:pr_lots_fixes branch 2 times, most recently from 7bcf977 to 36337d6 Dec 29, 2018

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2018

PTAL /cc @Araq @krux02

Didn't read through all the changes (too long), all I see is a decrease of the error message quality (seen in the diff of the test case). I don't like that.

that issue (as I mentioned above) was orhtogonal, and I've already fixed it in the meantime (see #10101)

Where are the test cases for the issues that you fixed?

done, see:

  • tests/errmsgs/t8794.nim
  • tests/errmsgs/t9768.nim

for #9270 and #9767 , not sure it's needed since it only affects --excessiveStackTrace:on and --listFullPaths ; if we do need a test case, please advise how to do test for messages that involve absolute paths in testament (I'm had suggested a few ways before using mocking technique (of absolute file), but @Araq was against it.

@timotheecour timotheecour force-pushed the timotheecour:pr_lots_fixes branch 2 times, most recently from 0ddfb66 to c660822 Dec 31, 2018

@timotheecour timotheecour requested review from krux02 and removed request for Araq Dec 31, 2018

@timotheecour timotheecour force-pushed the timotheecour:pr_lots_fixes branch from c660822 to 2288858 Dec 31, 2018

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

ping /cc @Araq @krux02

@timotheecour timotheecour changed the title lots of fixes around stacktraces and error messages [error messages] fix #8794 #9270 #9767 #9768 Jan 6, 2019

@timotheecour

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

ping /cc @Araq @krux02

@krux02

krux02 approved these changes Jan 8, 2019

Copy link
Contributor

left a comment

I am ok with the code, just the comments: no new todo, and line comments over block comments.

Show resolved Hide resolved compiler/semcall.nim

@timotheecour timotheecour merged commit bf3a308 into nim-lang:devel Jan 8, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@timotheecour timotheecour deleted the timotheecour:pr_lots_fixes branch Jan 9, 2019

timotheecour added a commit to timotheecour/Nim that referenced this pull request Jan 9, 2019

genotrance added a commit that referenced this pull request Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.