-
-
Notifications
You must be signed in to change notification settings - Fork 606
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
Implement -assert=[on|off] to enable/disable asserts on demand #6289
Conversation
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) |
There was a problem hiding this comment.
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.
I don't think this adds much of any value to
|
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:
but writing code that way is awkward and just doesn't look good. I propose instead to do something analogous to the accepted
Can you write a DIP for it? |
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. |
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. Regarding |
The issue itself (15962) needs another fix. |
FYI @MartinNowak @mihails-strasuns-sociomantic