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

make setting --backend:xxx also set exception handling strategy #14349

Closed
wants to merge 3 commits into from

Conversation

cooldome
Copy link
Member

@cooldome cooldome commented May 14, 2020

Currently nim check a.nim fails if a contains importing and handling of native cpp exceptions. nim check isn't aware of cpp exceptions. I thought nim check --backend:cpp will fix it, but this PR is required to make it work.

@Araq Araq added the merge_when_passes_CI mergeable once green label May 14, 2020
@Araq
Copy link
Member

Araq commented May 14, 2020

For some reason this fails:

tests/stdlib/tosproc.nim

@@ -444,7 +466,7 @@ proc processSwitch*(switch, arg: string, pass: TCmdLinePass, info: TLineInfo;
of "backend", "b":
let backend = parseEnum(arg.normalize, TBackend.default)
if backend == TBackend.default: localError(conf, info, "invalid backend: '$1'" % arg)
conf.backend = backend
handleBackend(conf, backend)
Copy link
Member

@timotheecour timotheecour May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this prevents doing:
nim r -b:c -b:cpp main
this is useful to provide a way to override a backend in an command line, eg:
you have a command line template nim r -b:c and then you simply add a flag to override via nim r -b:c -b:cpp

right now there's a bug (timotheecour#175) so this doesn't quite work yet (I can fix it unless you want to fix it), but your patch makes it worse because handleBackend has side effect. So we should fix both timotheecour#175 and nim check

TLDR: parsing the flag --backend should have zero side effect beyond setting conf.backend = backend, so that it can be overridden in subsequent flags

(eg, smthg similar is used for runnableExamples, where we have a template command that can be overriden with --doccmd:-b:cpp -d:foo)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely wrong:
parsing the flag --backend should have zero side effect beyond setting conf.backend =
backend

Copy link
Member

@timotheecour timotheecour May 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's by design so that it can be overridden and so that it plays nice with other commands; the side effects are deferred until the command is processed (eg nim doc etc).

I'm fixing this issue + others in #14363 which should supersede your PR

@timotheecour timotheecour removed the merge_when_passes_CI mergeable once green label May 16, 2020
@timotheecour
Copy link
Member

@cooldome ok to close this? (see #14363)

@cooldome
Copy link
Member Author

Great thanks, closing this PR

@cooldome cooldome closed this May 16, 2020
@narimiran narimiran deleted the backend_for_nimcheck branch August 4, 2020 07:37
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.

3 participants