Skip to content

Conversation

9il
Copy link
Member

@9il 9il commented Nov 26, 2019

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 26, 2019

Codecov Report

Merging #222 into master will decrease coverage by 1.16%.
The diff coverage is 74.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
- Coverage   92.19%   91.02%   -1.17%     
==========================================
  Files          44       49       +5     
  Lines        8368     8938     +570     
==========================================
+ Hits         7715     8136     +421     
- Misses        653      802     +149
Impacted Files Coverage Δ
source/mir/ndslice/slice.d 97.19% <ø> (ø) ⬆️
source/mir/container/binaryheap.d 84.32% <ø> (ø) ⬆️
source/mir/rc/context.d 71.11% <ø> (ø) ⬆️
source/mir/array/allocation.d 84.33% <100%> (+1.2%) ⬆️
source/mir/series.d 94.11% <100%> (ø) ⬆️
source/mir/format_impl.d 36.92% <36.92%> (ø)
source/mir/exception.d 62.06% <62.06%> (ø)
source/mir/algorithm/setops.d 91.17% <75%> (-8.83%) ⬇️
source/mir/appender.d 86.88% <86.88%> (ø)
source/mir/format.d 87.76% <87.76%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 476a4a3...8a0b94f. Read the comment docs.

@9il 9il merged commit 3aa93ea into master Nov 26, 2019
@9il 9il deleted the runtime branch November 26, 2019 07:58
@@ -20,9 +20,11 @@ buildType "unittest-release" {
versions "mir_test"
}

dflags "-preview=dip1008"

Choose a reason for hiding this comment

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

was this deliberate? It breaks anyone who can't use dip1008.

Also, dub doesn't propagate dflags to the whole build, only to packages that depend on the package that sets them, which means you end up building part of the build with dip1008 and part without. I noticed because linker errors where something was @nogc in the root package but not in its dependencies. So in general it just doesn't work to set dip flags in a library config.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Interesting is there a case when the DIP can't be used? I thought it is quite good.

I will create a patch tomorrow.

Choose a reason for hiding this comment

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

To be honest, there probably aren't many practical cases where you can't use it (maybe if you catch and store exceptions?), but nonetheless it does cause quite confusing breakage (because dub and dflags don't work well)

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