-
Notifications
You must be signed in to change notification settings - Fork 704
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
Control expensive assertion in Distribution.Solver.Modular.Linking with a build flag. #4346
Conversation
I added a function, 'debugAssert', that wraps 'assert' and only calls it when the build flag 'debug-assertions' is enabled. The flag defaults to false. I only replaced one call to 'assert' so far (in Distribution.Solver.Modular.Linking) in order to resolve haskell#4258.
cabal-install/cabal-install.cabal
Outdated
@@ -189,6 +189,11 @@ Flag network-uri | |||
description: Get Network.URI from the network-uri package | |||
default: True | |||
|
|||
Flag debug-assertions |
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.
Bikeshedding: what about introducing a flag assertions
and call this one expensive-assertions
to make it more clear what's going on? Then I guess debugAssert
could be renamed to expensiveAssert
. What do you think?
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.
Or, given that we already have debug-tracetree
, we could also have debug-assertions
for ghc-options: -fno-ignore-asserts
and debug-expensive-assertions
for expensive ones.
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.
I like the names debug-expensive-assertions
and expensiveAssert
. I think that it would be simpler to use ghc-options: -fno-ignore-asserts
to enable the usual assertions in cabal.project, though, because we could set the same flag for all packages:
Line 11 in 9348682
-- ghc-options: -fno-ignore-asserts |
Everything looks good to me, merging. |
Merged, thanks! |
Thanks! |
I added a function,
debugAssert
, that wrapsassert
and only calls it whenthe build flag
debug-assertions
is enabled. The flag defaults to false. Ionly replaced one call to
assert
so far (in Distribution.Solver.Modular.Linking)in order to resolve #4258.
I enabled
debug-assertions
in a new GHC 8.0.1 build job, because I figured that it is important to continue testing with the default flag settings in all of the other matrix entries. I could set-fdebug-assertions
in one of the existing jobs instead, if it's not worth creating a separate job.I can also make a PR for the 2.0 branch.