Skip to content

Revive #3531 - Adding support for __traits(documentation, ...)#6872

Closed
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:revive-3531
Closed

Revive #3531 - Adding support for __traits(documentation, ...)#6872
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:revive-3531

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Jun 8, 2017

So I took the liberty to revive #3531, but unfortunately there hasn't been a clear consensus on how to move forward. The most important bits from the discussion:

I object to turning this on by default [doDocComment = true] - it also affects memory consumption, which is currently a significant problem with dmd.

The discussion circled around adding a special flag for this as -D always produces an html file.

  • I think the newline should be stripped before returning it as a __traits(), otherwise user code will constantly have to strip it manually.

I share this opinion, but I wanted to wait on feedback before investing more work on this PR (same for adding more tests).

The PR to dlang.org is still open.

So three years later, what's your opinion on being able to query DDoc comments during CTFE?

@CyberShadow
Copy link
Member

it also affects memory consumption

Since you have an implementation, why not measure the memory consumption?

@andralex
Copy link
Member

andralex commented Jun 8, 2017

What would be some good use cases?

One simple way to estimate the impact on memory consumption is to build phobos with and without the feature enabled, and measure time and peak memory consumption. As phobos has profuse ddoc comments, that should be toward the upper bound. If there is no significant impact (as I suspect), then that particular argument goes away. @wilzbach can you to this test? Thanks!

@CyberShadow
Copy link
Member

What would be some good use cases?

Here is my use case:

https://github.com/CyberShadow/Digger/blob/72eb4718eb2eaf5d24f5cd724a0e3273a0ee4641/digger.d#L64-L158

Here, each method represents a command-line action. I use a slightly ugly string literal UDA for the --help documentation, but it would be nicer to be able to use regular DDoc comments instead.

@andralex
Copy link
Member

andralex commented Jun 8, 2017

@CyberShadow cool, though one counter-argument is that cmdline help documentation has restrictions (e.g. single-line, under 70 chars or so) that actually make it more suitable as a UDA. Documentation comments are usually not subjected to such restrictions.

Just to clarify the general idea: I'm generally in favor of more introspection capabilities, but each should be motivated by good applicability.

@PetarKirov
Copy link
Member

PetarKirov commented Jun 8, 2017

@andralex just replace cmdline with GUI - i.e. one function per GUI command; hovering over a button displays the documentation in a tooltip.

Another use case is named unittests. A test runner could show the name of a failing unittest with something along the lines of:

foreach (test; __traits(getUnitTests, MyClass))
    try
        test();
    catch (AssertError e)
    {
        static immutable summary = __traits(documentation, test);
"
+-------------------------------------
| %s
+-------------------------------------
| Summary: %s
| File: %s
| Line: %s
+-------------------------------------"
        .writefln(e.msg, summary, e.file, e.line);
        continue;
    }

Also, one could easily write a mixin statically asserts that all public functions have documentation and also that functions​ with non-void return types have a "Returns: ..." section, etc.

@PetarKirov
Copy link
Member

One could also imagine using CTFE instead of ddoc macros for certain use cases, or even preprocessing documentation comments, mixing them again and leaving them to the standard ddoc pipeline.

@andralex
Copy link
Member

andralex commented Jun 8, 2017

One could also imagine using CTFE instead of ddoc macros for certain use cases, or even preprocessing documentation comments, mixing them again and leaving them to the standard ddoc pipeline.

Whoa, wait. Are you saying a module could define and run a compile-time function that preprocesses some or all of the documentation in that module? That would be solid!

@PetarKirov
Copy link
Member

Yes, exactly my idea, though I haven't tried it yet.

@PetarKirov
Copy link
Member

PetarKirov commented Jun 8, 2017

I tried the following function:

import std.algorithm, std.range, std.format, std.meta, std.traits;
enum stringOf(T) = T.stringof;
enum sizeOf(T) = T.sizeof;
string makeDdoc(alias f)(string summary)
{
    return format(
"
/**
 * %s
 * 
 * Params:
%-(%s\n%)
 */",
        summary,
        [ ParameterIdentifierTuple!f ]
            .zip([ staticMap!(stringOf, Parameters!f) ], [ staticMap!(sizeOf, Parameters!f) ])
            .map!(x => " *     %s = A parameter of type `%s`, size %s.".format(x[0], x[1], x[2]))
    );
}

That when applied to a function fun:

void fun(short x, float y, double z);

makeDdoc!fun("This is a great function.")

Yields:

/**
 * This is a great function.
 *
 * Params:
 *     x = A parameter of type `short`, size 2.
 *     y = A parameter of type `float`, size 4.
 *     z = A parameter of type `double`, size 8.
 */

However, when I mixed in the documentation in front of the function like so:

mixin (makeDdoc!fun("This is a great function."));
void fun(short x, float y, double z);

And ran the following command:

dmd -D test.d

I got:
actual
When I was expecting:
expected

Which confirmed my expectation that ddoc parsing doesn't take into account mixins.
@andralex do you think there is value in opening an enhancement request for adding a flag to dmd which would force the evaluation of all mixins at module scope before ddoc is parsed?

@andralex
Copy link
Member

andralex commented Jun 8, 2017

I'd say this PR would be a lot more valuable if it afforded users the ability to preprocess ddoc comments.

@PetarKirov
Copy link
Member

PetarKirov commented Jun 8, 2017

On second thought, I don't think we need much more than more than __traits(documentation, symbol), because preprocessing can easily be accomplished with a little more work:

dmd -o- test.d 2> preprocessed.d && dmd -o- -Dftest.html -D preprocessed.d && rm preprocessed.d

test.d:

import std.algorithm, std.array, std.range, std.format, std.meta, std.traits;

enum stringOf(T) = T.stringof;
enum sizeOf(T) = T.sizeof;
string makeDdoc(alias f)(string summary)
{
    return format(
"
/**
 * %s
 * 
 * Params:
%-(%s\n%)
 */
%s;",
        summary,
        [ ParameterIdentifierTuple!f ]
            .zip([ staticMap!(stringOf, Parameters!f) ], [ staticMap!(sizeOf, Parameters!f) ])
            .map!(x => " *     %s = A parameter of type `%s`, size %s.".format(x[0], x[1], x[2])),
        typeof(&f).stringof.replace("function", __traits(identifier, f))
    );
}

pragma (msg, makeDdoc!fun("This is a great function."));
void fun(short x, float y, double z);

test.html:

preprocessed

Replacing the pragma (msg, ...) with a mixin that iterates over the members of the module should be good enough for a lot of use cases.

@wilzbach
Copy link
Contributor Author

wilzbach commented Jun 11, 2017

One simple way to estimate the impact on memory consumption is to build phobos with and without the feature enabled, and measure time and peak memory consumption. As phobos has profuse ddoc comments, that should be toward the upper bound. If there is no significant impact (as I suspect), then that particular argument goes away. @wilzbach can you to this test? Thanks!

So we allocate 2.75 MB more at the peak, which imho would be neglectable considering that the peak is at 1.1 GB.
However there seems to be a noticable difference in avgtime, but better see the raw numbers for yourself.

With __traits(documentation)

Peak memory consumption: (i.e. last Valgrind massif snapshot):

--------------------------------------------------------------------------------
  n        time(i)         total(B)   useful-heap(B) extra-heap(B)    stacks(B)
--------------------------------------------------------------------------------
 70 23,355,923,847    1,253,708,328    1,159,290,142    94,418,186            0

image

massif_doc

AvgTime

> ~/dlang/avgtime/avgtime -r30 -d -q $(cat test.sh)
------------------------
Total time (ms): 200455
Repetitions    : 30
Sample mode    : 6730 (2 occurrences)
Median time    : 6443.3
Avg time       : 6681.82
Std dev.       : 555.537
Minimum        : 6047.94
Maximum        : 8455.6
95% conf.int.  : [5592.98, 7770.65]  e = 1088.83
99% conf.int.  : [5250.85, 8112.79]  e = 1430.97
EstimatedAvg95%: [6483.02, 6880.61]  e = 198.793
EstimatedAvg99%: [6420.56, 6943.08]  e = 261.258

Without __traits(documentation)

Peak memory consumption:

--------------------------------------------------------------------------------
  n        time(i)         total(B)   useful-heap(B) extra-heap(B)    stacks(B)
--------------------------------------------------------------------------------
 87 23,227,888,304    1,250,958,184    1,156,655,280    94,302,904            0

image

massif

AvgTime

> ~/dlang/avgtime/avgtime -r30 -d -q $(cat test.sh)
Total time (ms): 197151
Repetitions    : 30
Sample mode    : 5860 (2 occurrences)
Median time    : 6398.85
Avg time       : 6571.69
Std dev.       : 716.995
Minimum        : 5788.27
Maximum        : 7886.89
95% conf.int.  : [5166.41, 7976.98]  e = 1405.28
99% conf.int.  : [4724.84, 8418.55]  e = 1846.86
EstimatedAvg95%: [6315.12, 6828.26]  e = 256.569
EstimatedAvg99%: [6234.5, 6908.88]  e = 337.188

Final

Interestingly, when run through valgrind an assert in std/math.d fails:

+++ b/std/math.d
@@ -3806,7 +3806,7 @@ real hypot(real x, real y) @safe pure nothrow @nogc
     static assert(2*(SQRTMAX/2)*(SQRTMAX/2) <= real.max);
 
     // Proves that sqrt(real.max) ~~  0.5/sqrt(real.min_normal)
-    static assert(real.min_normal*real.max > 2 && real.min_normal*real.max <= 4);
+    //static assert(real.min_normal*real.max > 2 && real.min_normal*real.max <= 4);
 
     real u = fabs(x);
     real v = fabs(y);

@andralex
Copy link
Member

Yah, the increase in build time is measurable. My main problem is we're lacking a killer use case for this. In fact the converse may be more useful: make the body of a enum or function available to ddoc. Then you can click on a "+" to expand it inline.

@wilzbach
Copy link
Contributor Author

Yah, the increase in build time is measurable. My main problem is we're lacking a killer use case for this. In fact the converse may be more useful: make the body of a enum or function available to ddoc. Then you can click on a "+" to expand it inline.

Please don't let us develop no features for Ddoc - this is far easier with Ddox.
In fact we already have a "View source code" button which takes you to the exact lines on the source code, e.g. https://dlang.org/library/std/algorithm/comparison/among.html

So what other options do we have?

  • Only activate when used with -D
  • Only activate with a special flag
  • Dump the idea

@andralex
Copy link
Member

I'd say keep it on the back burner until we find a couple of good ideas for using it. Discussing it in the forum may help.

@wilzbach
Copy link
Contributor Author

I'd say keep it on the back burner until we find a couple of good ideas for using it. Discussing it in the forum may help.

-> http://forum.dlang.org/post/cwjtsmnwwwwepvgvlqcy@forum.dlang.org

@adamdruppe
Copy link
Contributor

I like cybershadow's use case - loading the documentation comment is really nice for command line things. Column limits are a simple case of authoring and word-wrapping; trivial, no need to separate and duplicate the text.

As to preprocessing docs... no, bad idea. That'd be something the alternative doc parsers can't really duplicate and would make the docs likely less readable in source code. That's a net negative value.

@tchaloupka
Copy link
Contributor

One usecase I would add is compile time generation of the OpenAPI documentation of REST API (ie using vibe-d) - see https://swagger.io/

@stefan-koch-sociomantic

Although I cannot see a immediate usecase, I will say that at this introducing memory problems is no concern.
So technically this will fly like a bird.

@WalterBright WalterBright added the Review:Phantom Zone Has value/information for future work, but closed for now label Jan 18, 2018
@WalterBright
Copy link
Member

Putting this in the Phantom Zone due it needing a compelling use case(s) to justify consuming memory and compilation time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review:Needs Rebase Review:Needs Work Review:Phantom Zone Has value/information for future work, but closed for now Review:stalled

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants