Skip to content

Enable runnable examples - remove the last modules from the blacklist #5607

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.

Sign up for GitHub

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

Merged
dlang-bot merged 7 commits into from
Oct 7, 2017
Merged

Enable runnable examples - remove the last modules from the blacklist #5607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2e96d06
Issue 16984 - Make std.math runnable
wilzbach Jul 12, 2017
c2d03bf
Issue 16984 - Make std.stdio runnable
wilzbach Jul 12, 2017
3cc6300
Issue 16984 - Make std.traits runnable
wilzbach Jul 12, 2017
ff6c0f1
Issue 16984 - Make stdx.allocator.building_blocks.quantizer runnable
wilzbach Jul 12, 2017
5a20f69
Issue 16984 - Make stdx.allocator.building_blocks.free_list runnable
wilzbach Jul 12, 2017
d661120
Fix Issue 16984 - Remove runnable blacklist
wilzbach Jul 12, 2017
41de215
Remove invalid private access by getSymbolsByUDA from unittest
wilzbach Oct 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions posix.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,15 +264,6 @@ MAKEFILE = $(firstword $(MAKEFILE_LIST))
# build with shared library support (defaults to true on supported platforms)
SHARED=$(if $(findstring $(OS),linux freebsd),1,)

# Check for missing imports in public unittest examples.
# A blacklist of ignored module is provided as not all public unittest in
# Phobos are independently runnable yet
IGNORED_PUBLICTESTS= $(addprefix std/, \
$(addprefix experimental/allocator/, \
building_blocks/free_list building_blocks/quantizer \
) \
math stdio traits)
PUBLICTESTS= $(addsuffix .publictests,$(filter-out $(IGNORED_PUBLICTESTS), $(D_MODULES)))
TESTS_EXTRACTOR=$(ROOT)/tests_extractor
PUBLICTESTS_DIR=$(ROOT)/publictests

Expand Down Expand Up @@ -593,7 +584,7 @@ style_lint: dscanner $(LIB)
################################################################################
# Check for missing imports in public unittest examples.
################################################################################
publictests: $(PUBLICTESTS)
publictests: $(addsuffix .publictests,$(D_MODULES))

$(TESTS_EXTRACTOR): $(TOOLS_DIR)/tests_extractor.d | $(LIB)
DFLAGS="$(DFLAGS) $(LIB) -defaultlib= -debuglib= $(LINKDL)" $(DUB) build --force --compiler=$${PWD}/$(DMD) --single $<
Expand Down
60 changes: 29 additions & 31 deletions std/experimental/allocator/building_blocks/free_list.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,7 @@ struct FreeList(ParentAllocator,
_max = high;
}

///
@safe unittest
@system unittest
Copy link
Member

@PetarKirov PetarKirov Oct 6, 2017

Choose a reason for hiding this comment

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

I wonder why it worked with @safe before?

Copy link
Contributor Author

@wilzbach wilzbach Oct 7, 2017

Choose a reason for hiding this comment

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

Because it was never executed, see the comment about unittest blocks in templates before. By moving the unittest to the module level, this unittest was executed for the first time...

Copy link
Member

@PetarKirov PetarKirov Oct 7, 2017

Choose a reason for hiding this comment

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

Quite unfortunate. Thanks for the clarification.

{
import std.experimental.allocator.common : chooseAtRuntime;
import std.experimental.allocator.mallocator : Mallocator;
Expand Down Expand Up @@ -895,42 +894,13 @@ struct SharedFreeList(ParentAllocator,
@property void max(size_t newMaxSize);
/// Ditto
void setBounds(size_t newMin, size_t newMax);
///
@safe unittest
{
import std.experimental.allocator.common : chooseAtRuntime;
import std.experimental.allocator.mallocator : Mallocator;

shared SharedFreeList!(Mallocator, chooseAtRuntime, chooseAtRuntime) a;
// Set the maxSize first so setting the minSize doesn't throw
a.max = 128;
a.min = 64;
a.setBounds(64, 128); // equivalent
assert(a.max == 128);
assert(a.min == 64);
}
Copy link
Member

@PetarKirov PetarKirov Oct 6, 2017

Choose a reason for hiding this comment

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

What's the reason for moving the unit test to module level? Is it because the test extractor cannot handle unit tests for member functions? I don't like this change as now the example won't show immediately after the function, but on rather different place (especially with the ddox layout).

Copy link
Contributor Author

@wilzbach wilzbach Oct 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

A unittest in a template will be part of every instantiation, they shouldn't have been there in the first place.See DIP82 for a more detailed description of the problem.
Also note that the unittest will still be associated with its struct.

Copy link
Member

@PetarKirov PetarKirov Oct 7, 2017

Choose a reason for hiding this comment

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

This can be worked around via static if (doUnittest) (see e.g. https://github.com/libmir/mir-algorithm/blob/e590f7f46058588f1ceb08672c92bd6248863e18/source/mir/ndslice/slice.d#L737).
Essentially you need to make sure that doUnittest is true for a single set of template arguments and that you have a template instance with those arguments (e.g. used in a private unittest).

Copy link
Member

@PetarKirov PetarKirov Oct 7, 2017

Choose a reason for hiding this comment

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

(If you don't want to deal with this now, we can leave it for later.)

Copy link
Contributor Author

@wilzbach wilzbach Oct 7, 2017

Choose a reason for hiding this comment

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

This can be worked around via static if (doUnittest)

Considering that the test didn't compile until now (in case you didn't notice, the moved out version is different - it doesn't set the bounds twice as this is prohibited), I was merely trying to get things to work, s.t. we can finally move forward with the runnable examples ;-)
Regarding the static if (doUnittest): I am aware of this pattern (even the old std.experimental.ndslice used this), but AFAIK that has been the only occurrence of this pattern in Phobos.
Anyways, there are still plenty of unittests in templated structs/classes, so I would strongly prefer if such a general cleanup is done in a later PR...

Copy link
Member

@PetarKirov PetarKirov Oct 7, 2017

Choose a reason for hiding this comment

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

Fair enough


/**
Properties for getting (and possibly setting) the approximate maximum length of a shared freelist.
*/
@property size_t approxMaxLength() const shared;
/// ditto
@property void approxMaxLength(size_t x) shared;
///
@safe unittest
{
import std.experimental.allocator.common : chooseAtRuntime;
import std.experimental.allocator.mallocator : Mallocator;

shared SharedFreeList!(Mallocator, 50, 50, chooseAtRuntime) a;
// Set the maxSize first so setting the minSize doesn't throw
a.approxMaxLength = 128;
assert(a.approxMaxLength == 128);
a.approxMaxLength = 1024;
assert(a.approxMaxLength == 1024);
a.approxMaxLength = 1;
assert(a.approxMaxLength == 1);
}
Copy link
Member

@PetarKirov PetarKirov Oct 6, 2017

Choose a reason for hiding this comment

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

ditto

}

/**
Expand Down Expand Up @@ -1071,6 +1041,34 @@ struct SharedFreeList(ParentAllocator,
}
}

///
@safe unittest
{
import std.experimental.allocator.common : chooseAtRuntime;
import std.experimental.allocator.mallocator : Mallocator;

shared SharedFreeList!(Mallocator, chooseAtRuntime, chooseAtRuntime) a;
a.setBounds(64, 128);
assert(a.max == 128);
assert(a.min == 64);
}

///
@safe unittest
{
import std.experimental.allocator.common : chooseAtRuntime;
import std.experimental.allocator.mallocator : Mallocator;

shared SharedFreeList!(Mallocator, 50, 50, chooseAtRuntime) a;
// Set the maxSize first so setting the minSize doesn't throw
a.approxMaxLength = 128;
assert(a.approxMaxLength == 128);
a.approxMaxLength = 1024;
assert(a.approxMaxLength == 1024);
a.approxMaxLength = 1;
assert(a.approxMaxLength == 1);
}

@system unittest
{
import core.thread : ThreadGroup;
Expand Down
9 changes: 7 additions & 2 deletions std/experimental/allocator/building_blocks/quantizer.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,14 +212,19 @@ struct Quantizer(ParentAllocator, alias roundingFunction)
@system unittest
{
import std.experimental.allocator.building_blocks.free_tree : FreeTree;
import std.experimental.allocator.common : roundUpToMultipleOf;
import std.experimental.allocator.gc_allocator : GCAllocator;

size_t roundUpToMultipleOf(size_t s, uint base)
{
auto rem = s % base;
return rem ? s + base - rem : s;
}

// Quantize small allocations to a multiple of cache line, large ones to a
// multiple of page size
alias MyAlloc = Quantizer!(
FreeTree!GCAllocator,
n => n.roundUpToMultipleOf(n <= 16_384 ? 64 : 4096));
n => roundUpToMultipleOf(n, n <= 16_384 ? 64 : 4096));
MyAlloc alloc;
const buf = alloc.allocate(256);
assert(buf.ptr);
Expand Down
17 changes: 15 additions & 2 deletions std/math.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2771,8 +2771,7 @@ if (isFloatingPoint!T)
int exp;
real mantissa = frexp(123.456L, exp);

// check if values are equal to 19 decimal digits of precision
assert(equalsDigit(mantissa * pow(2.0L, cast(real) exp), 123.456L, 19));
assert(approxEqual(mantissa * pow(2.0L, cast(real) exp), 123.456L));
Copy link
Member

@ibuclaw ibuclaw Jul 13, 2017

Choose a reason for hiding this comment

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

I'd have to check this on a target that demanded it.

Copy link
Contributor Author

@wilzbach wilzbach Jul 15, 2017

Choose a reason for hiding this comment

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

You don't need to, I moved the test to an internal unittest. This change is just needed because approxEqual isn't a public symbol and would error when run online (or wherever the user tries to run the example).

Copy link
Member

@ibuclaw ibuclaw Jul 15, 2017

Choose a reason for hiding this comment

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

Yes, but does it pass on ARM? Or any other real==double target for that matter?

If equalsDigit and approxEqual are sufficiently the same, then there's no problem. :)

Copy link
Contributor Author

@wilzbach wilzbach Jul 17, 2017

Choose a reason for hiding this comment

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

If equalsDigit and approxEqual are sufficiently the same, then there's no problem. :)

approxEqual (used for the public examples) is much weaker, than equalsDigit. approxEqual checks by a difference of 1e-5 by default, whereas the equalsDigit tests here are by a precision of 1e-19.


assert(frexp(-real.nan, exp) && exp == int.min);
assert(frexp(real.nan, exp) && exp == int.min);
Expand All @@ -2782,6 +2781,15 @@ if (isFloatingPoint!T)
assert(frexp(0.0, exp) == 0.0 && exp == 0);
}

@system unittest
{
int exp;
real mantissa = frexp(123.456L, exp);

// check if values are equal to 19 decimal digits of precision
assert(equalsDigit(mantissa * pow(2.0L, cast(real) exp), 123.456L, 19));
}

@safe unittest
{
import std.meta : AliasSeq;
Expand Down Expand Up @@ -3619,6 +3627,11 @@ real log2(real x) @safe pure nothrow @nogc
}

///
@system unittest
{
assert(approxEqual(log2(1024.0L), 10));
Copy link
Member

@ibuclaw ibuclaw Jul 13, 2017

Choose a reason for hiding this comment

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

Actually, why are you testing with both approxEqual() and equalsDigit(19)?

Copy link
Contributor Author

@wilzbach wilzbach Jul 13, 2017

Choose a reason for hiding this comment

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

equalsDigit is private, that's why I copied the test to a separate unittest and inserted approxEqual instead

}

@system unittest
{
// check if values are equal to 19 decimal digits of precision
Expand Down
10 changes: 5 additions & 5 deletions std/stdio.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +930,7 @@ $(D rawRead) always reads in binary mode on Windows.
{
static import std.file;

auto testFile = testFilename();
auto testFile = std.file.deleteme();
Copy link
Contributor Author

@wilzbach wilzbach Jul 12, 2017

Choose a reason for hiding this comment

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

Not a huge fan of this, but deleteme is the "defacto" standard within Phobos and used in many other public examples.

std.file.write(testFile, "\r\n\n\r\n");
scope(exit) std.file.remove(testFile);

Expand Down Expand Up @@ -985,7 +985,7 @@ Throws: $(D ErrnoException) if the file is not opened or if the call to $(D fwri
{
static import std.file;

auto testFile = testFilename();
auto testFile = std.file.deleteme();
auto f = File(testFile, "w");
scope(exit) std.file.remove(testFile);

Expand Down Expand Up @@ -1092,7 +1092,7 @@ Throws: $(D Exception) if the file is not opened.
import std.conv : text;
static import std.file;

auto testFile = testFilename();
auto testFile = std.file.deleteme();
std.file.write(testFile, "abcdefghijklmnopqrstuvwqxyz");
scope(exit) { std.file.remove(testFile); }

Expand Down Expand Up @@ -1839,7 +1839,7 @@ $(CONSOLE
{
static import std.file;

auto deleteme = testFilename();
auto deleteme = std.file.deleteme();
std.file.write(deleteme, "hello\nworld\ntrue\nfalse\n");
scope(exit) std.file.remove(deleteme);
string s;
Expand Down Expand Up @@ -2491,7 +2491,7 @@ $(REF readText, std,file)
import std.typecons : tuple;

// prepare test file
auto testFile = testFilename();
auto testFile = std.file.deleteme();
scope(failure) printf("Failed test at line %d\n", __LINE__);
std.file.write(testFile, "1 2\n4 1\n5 100");
scope(exit) std.file.remove(testFile);
Expand Down
23 changes: 15 additions & 8 deletions std/traits.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2490,6 +2490,7 @@ template Fields(T)
///
@safe unittest
{
import std.meta : AliasSeq;
struct S { int x; float y; }
static assert(is(Fields!S == AliasSeq!(int, float)));
}
Expand Down Expand Up @@ -2546,6 +2547,7 @@ template FieldNameTuple(T)
///
@safe unittest
{
import std.meta : AliasSeq;
struct S { int x; float y; }
static assert(FieldNameTuple!S == AliasSeq!("x", "y"));
static assert(FieldNameTuple!int == AliasSeq!"");
Expand Down Expand Up @@ -2692,7 +2694,7 @@ private template hasRawAliasing(T...)
enum hasRawAliasing = Impl!(RepresentationTypeTuple!T);
}

///
//
Copy link
Member

@CyberShadow CyberShadow Sep 14, 2017

Choose a reason for hiding this comment

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

Why are we undocumenting useful example blocks?

Copy link
Contributor Author

@wilzbach wilzbach Oct 5, 2017

Choose a reason for hiding this comment

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

Because hasRawAliasing is a private symbol -> https://github.com/dlang/phobos/blob/v2.076.0/std/traits.d#L2661

Copy link
Member

@CyberShadow CyberShadow Oct 5, 2017

Choose a reason for hiding this comment

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

Oh, OK. So this was an "example" for a private symbol.

@safe unittest
{
// simple types
Expand Down Expand Up @@ -2788,7 +2790,7 @@ private template hasRawUnsharedAliasing(T...)
enum hasRawUnsharedAliasing = Impl!(RepresentationTypeTuple!T);
}

///
//
@safe unittest
{
// simple types
Expand Down Expand Up @@ -4009,6 +4011,8 @@ template BaseTypeTuple(A)
///
@safe unittest
{
import std.meta : AliasSeq;

interface I1 { }
interface I2 { }
interface I12 : I1, I2 { }
Expand Down Expand Up @@ -4062,6 +4066,8 @@ template BaseClassesTuple(T)
///
@safe unittest
{
import std.meta : AliasSeq;

class C1 { }
class C2 : C1 { }
class C3 : C2 { }
Expand Down Expand Up @@ -4422,6 +4428,8 @@ template TemplateArgsOf(T : Base!Args, alias Base, Args...)
///
@safe unittest
{
import std.meta : AliasSeq;

struct Foo(T, U) {}
static assert(is(TemplateArgsOf!(Foo!(int, real)) == AliasSeq!(int, real)));
}
Expand Down Expand Up @@ -7331,6 +7339,8 @@ template mostNegative(T)
///
@safe unittest
{
import std.meta : AliasSeq;

foreach (T; AliasSeq!(bool, byte, short, int, long))
static assert(mostNegative!T == T.min);

Expand Down Expand Up @@ -7397,6 +7407,7 @@ template mangledName(sth...)
///
@safe unittest
{
import std.meta : AliasSeq;
alias TL = staticMap!(mangledName, int, const int, immutable int);
static assert(TL == AliasSeq!("i", "xi", "yi"));
}
Expand Down Expand Up @@ -7851,18 +7862,14 @@ template getSymbolsByUDA(alias symbol, alias attribute)
enum Attr;
struct A
{
alias int INT;
alias INT = int;
alias void function(INT) SomeFunction;
@Attr int a;
int b;
@Attr private int c;
private int d;
}

// Here everything is fine, we have access to private member c
static assert(getSymbolsByUDA!(A, Attr).length == 2);
static assert(getSymbolsByUDA!(A, Attr).length == 1);
static assert(hasUDA!(getSymbolsByUDA!(A, Attr)[0], Attr));
static assert(hasUDA!(getSymbolsByUDA!(A, Attr)[1], Attr));
}

// #16387: getSymbolsByUDA works with structs but fails with classes
Expand Down