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

Implement -assert=[on|off] to enable/disable asserts on demand #6289

Closed
wants to merge 1 commit into from
Closed

Implement -assert=[on|off] to enable/disable asserts on demand #6289

wants to merge 1 commit into from

Conversation

mathias-lang-sociomantic
Copy link
Contributor

@Geod24
Copy link
Member

Geod24 commented Nov 28, 2016

Hum, silly me - That can't work because of bootstrapping. @MartinNowak : How are you compiling the releases ? Could we do this in 2 steps, by first introducing this switch, and enabling it later on ?

@@ -358,20 +359,24 @@ private int tryMain(size_t argc, const(char)** argv)
getenv_setargv(readFromEnv(&environment, "DFLAGS"), &arguments);
updateRealEnvironment(&environment);
environment.reset(1); // don't need environment cache any more
version (none)
Copy link
Member

Choose a reason for hiding this comment

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

This is for debugging purposes. Please leave it in.

@WalterBright
Copy link
Member

I don't think this adds much of any value to -release. Note that -release has the following effects:

if (global.params.release)
{
    global.params.useInvariants = false;
    global.params.useIn = false;
    global.params.useOut = false;
    global.params.useAssert = false;
    global.params.useSwitchError = false;
}
if (global.params.useUnitTests)
    global.params.useAssert = true;

@WalterBright
Copy link
Member

It's also becoming clear that turning asserts on and off with the command line is a bit blunt, especially when projects tend to be all files on one command line. Some asserts are more equal than others. Individual asserts can be left on with:

if (!condition) assert(0);

but writing code that way is awkward and just doesn't look good.

I propose instead to do something analogous to the accepted pragma(inline, true/false), such as:

pragma(assert); // default
pragma(assert, true); // turn asserts on
pragma(assert, false); // turn asserts off

Can you write a DIP for it?

@mathias-lang-sociomantic
Copy link
Contributor Author

I don't think this adds much of any value to -release.

Yeah, we could allow more fine-grained control over what is enabled / disabled, but in any case, it won't solve the problem it's trying to solve, since we need to compile with the oldest C++ DMD.
Instead of changing D behavior, I'm considering just replacing all assert calls with throw new IceError()... Would that work, or do you have other suggestions ?

@WalterBright
Copy link
Member

Would that work

I don't see how it would be any different.

-release enables several checks, as I listed above. Probably a decent step is to try turning them on/off individually, and see which one contributes most to the 8% slowdown.

@mathias-lang-sociomantic
Copy link
Contributor Author

I don't see how it would be any different.

Well the problem it would introduce is that we won't be able to use this new feature directly for a while.
And as we keep compatibility with the last C++ DMD, it only makes matter worse.
Using an Error type would solve that issue.

Regarding pragma(assert), I'm not sure I understand your position completely.
It would allow one function (/ module / aggregate) to force assert to stay, right ? The point of this switch is to unconditionally do that for all module, rather than allowing fine-grained control (which is already possible by other means, such as the assert(0) trick you described, or a manual throw Error).

@mathias-lang-sociomantic mathias-lang-sociomantic changed the title Fix issue 15962 - Don't strip off asserts to check internal compiler errors Implement -assert=[on|off] to enable/disable asserts on demand Dec 21, 2016
@mathias-lang-sociomantic
Copy link
Contributor Author

The issue itself (15962) needs another fix.
The functionality (more control over assert than what debug / release provide) will be covered by dlang/DIPs#54

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