Skip to content

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Feb 4, 2018

I wish all segfaults would be that easy.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close Bugzilla Description
18367 dmd should not segfault on -X with libraries, but no source files

@wilzbach wilzbach added the Review:Trivial typos, formatting, comments label Feb 4, 2018
}
else
{
if (global.params.objfiles.dim == 0)
Copy link
Member

Choose a reason for hiding this comment

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

@wilzbach indentation seems to suggest you missed braces.

Copy link
Member

@UplinkCoder UplinkCoder left a comment

Choose a reason for hiding this comment

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

LGTM

@wilzbach
Copy link
Contributor Author

wilzbach commented Feb 4, 2018

Argh this is failing on Windows :/

@timotheecour
Copy link
Contributor

#7838 will supersede this behavior right? (but still good to submit in meantime)

@wilzbach
Copy link
Contributor Author

wilzbach commented Feb 4, 2018

I thought you don't want -X to emit JSON, but just -Xf=-?

@timotheecour
Copy link
Contributor

ya, sorry ignore my comment

@marler8997
Copy link
Contributor

marler8997 commented Feb 5, 2018

PR #7838 addresses this (I had to since I have a test for it). I defaulted to writing to info.json, however, your approach of asserting an error works as well. Let me know what you want to do.

@wilzbach
Copy link
Contributor Author

wilzbach commented Feb 6, 2018

I think producing an error is saner then writing to a default info.json file. This would just need extra documentation and the case isn't very popular anyways.

@marler8997
Copy link
Contributor

marler8997 commented Feb 6, 2018

Ok I changed my PR to issue an error in this case:

Error: cannot determine JSON filename, use -Xf=<file> or provide a source file

@@ -0,0 +1,11 @@
#!/usr/bin/env bash
Copy link
Contributor

@marler8997 marler8997 Feb 6, 2018

Choose a reason for hiding this comment

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

this whole file is bash wizadry...I can see one is strong with the bash

Copy link
Contributor

Choose a reason for hiding this comment

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

I copied this test to my PR, and added another case to it

@timotheecour
Copy link
Contributor

I defaulted to writing to info.json, however, your approach of asserting an error works as well. Let me know what you want to do.

indeed, IMO as a general rule it's always better to be explicit (let user choose file name or give error); it reduces bugs (eg what if a user already had an info.json and didn't intend it to be deleted)

@wilzbach
Copy link
Contributor Author

wilzbach commented Feb 9, 2018

Closing this in favor of #7838 then. Thanks @marler8997 !

@wilzbach wilzbach closed this Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review:Trivial typos, formatting, comments Severity:Bug Fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants