Skip to content
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

spec update following allowing default valued parameters after template variadics #2169

Merged
merged 1 commit into from
Feb 24, 2018

Conversation

timotheecour
Copy link
Contributor

@timotheecour timotheecour commented Feb 4, 2018

See also: dlang/dmd#7831

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 4, 2018

Thanks for your pull request and interest in making D better, @timotheecour! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

spec/template.dd Outdated
)

---
void $(CODE_HIGHLIGHT fun)(T...)(T t, string file = __FILE__)
Copy link
Member

Choose a reason for hiding this comment

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

No. We have automatic code highlighting. Also please wrap this in:

$(SPEC_RUNNABLE_EXAMPLE_COMPILE
---
...
---
)

so that the spec tester can find it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

spec/template.dd Outdated
@@ -1082,6 +1082,19 @@ $(H2 $(LNAME2 function-templates, Function Templates))
}
---

$(P Variadic Function templates can have parameters with default values.
Copy link
Member

Choose a reason for hiding this comment

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

Either "Variadic Function Templates" or "Variadic function templates".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@timotheecour
Copy link
Contributor Author

PTAL

spec/template.dd Outdated
}
fun(1, "foo"); // T.length = 2, not 1 (IFTI)
fun!int(1, "foo"); // T.length = 1
---
Copy link
Member

Choose a reason for hiding this comment

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

you need to close the ddoc macro here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

this always print 1 for me:

image

https://run.dlang.io/is/CKBf6g

Copy link
Contributor Author

@timotheecour timotheecour Feb 10, 2018

Choose a reason for hiding this comment

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

works from git master; probably run.dlang.io is running latest stable instead of master

Copy link
Member

@wilzbach wilzbach Feb 10, 2018

Choose a reason for hiding this comment

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

No it uses the the DMD nightlies and updates itself daily.
Though I think that's the root of the problem:

~/dlang/install.sh dmd-nightly
Downloading and unpacking http://nightlies.dlang.org/dmd-master-2018-02-01/dmd.master.linux.tar.xz

And yeah - there's a build error: http://nightlies.dlang.org/dmd-master-2018-02-10/build.html
Why is there no loud bot in place that complains about this? :/

edit: for reference, the build error has been fixed in #2200

Copy link
Member

Choose a reason for hiding this comment

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

Anyhow, maybe we should use _RUN + assert here? Then the spec tester will work like a testsuite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@wilzbach wilzbach changed the title spec update following https://github.com/dlang/dmd/pull/7831 spec update following allowing default valued parameters after template variadics Feb 10, 2018
@jacob-carlborg
Copy link
Contributor

Does this feature work for non-template variadic parameters? If that's the case, then I think there should be examples for that as well.

@timotheecour
Copy link
Contributor Author

no, only template variadics are supported for now; would be nice to support other variadics in future PRs.

@@ -1082,6 +1082,23 @@ $(H2 $(LNAME2 function-templates, Function Templates))
}
---

$(P Variadic Function Templates can have parameters with default values.
These parameters are always set to their default value in case of IFTI.
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgive my ignorance, but what is IFTI?

Copy link
Contributor

Choose a reason for hiding this comment

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

Implicit Function Template Instantiation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Found it here: https://wiki.dlang.org/Commonly-Used_Acronyms Maybe spell it out in the description, with the abbreviation in parens, and then use the abbreviation in the comments of the example.

Copy link
Contributor Author

@timotheecour timotheecour Feb 12, 2018

Choose a reason for hiding this comment

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

spec/template.dd Outdated
}
assert(fun(1, "foo") == 2); // not 1 (IFTI)
assert(fun!int(1, "foo") == 1); // IFTI
---
Copy link
Member

Choose a reason for hiding this comment

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

collect2: fatal error: ld terminated with signal 11 [Segmentation fault], core dumped

So that's weird. It compiles fine on my machine though with the test_dspect target I spuriously get the core dump.

gdb says:

Thread 6 (Thread 0x7ffff4e8f700 (LWP 13237)):
#0  0x00007ffff7bca73c in __lll_lock_wait () from /usr/lib/libpthread.so.0
#1  0x00007ffff7bc3ac6 in pthread_mutex_lock () from /usr/lib/libpthread.so.0
#2  0x00005555556ad179 in _D4core4sync5mutex5Mutex__T12lock_nothrowTCQBpQBnQBlQBiZQBdMFNbNiNeZv ()
#3  0x00005555556ad09d in core.sync.mutex.Mutex.lock() ()
#4  0x0000555555664f8f in _D3std11parallelism__T15ParallelForeachTSQBn9algorithm9iteration__T12FilterResultS_DQDe4file10dirEntriesFAyaQdEQEfQBb8SpanModebZ1fMFNaNbNfSQFhQCd8DirEntryZbTSQGaQCw11DirIteratorZQEhZQGe7opApplyMFMDFQCiZiZ4doItMFZ8makeTempMFZm (this=0x7ffff4e8e9f8)
    at /home/seb/dlang/phobos/std/parallelism.d-mixin-4011:4110
#5  0x0000555555664d64 in _D3std11parallelism__T15ParallelForeachTSQBn9algorithm9iteration__T12FilterResultS_DQDe4file10dirEntriesFAyaQdEQEfQBb8SpanModebZ1fMFNaNbNfSQFhQCd8DirEntryZbTSQGaQCw11DirIteratorZQEhZQGe7opApplyMFMDFQCiZiZ4doItMFZv (this=0x7fffffffd290)
    at /home/seb/dlang/phobos/std/parallelism.d-mixin-4011:4153
#6  0x00005555556b995c in _D3std11parallelism__T3runTDFZvZQkFQiZv ()
#7  0x00005555556b9498 in _D3std11parallelism__T4TaskSQBaQz3runTDFZvZQv4implFPvZv ()
#8  0x00005555556eef67 in std.parallelism.AbstractTask.job() ()
#9  0x00005555556b808c in _D3std11parallelism8TaskPool5doJobMFPSQBkQBj12AbstractTaskZv ()
#10 0x00005555556b81ea in std.parallelism.TaskPool.executeWorkLoop() ()
#11 0x00005555556b8191 in std.parallelism.TaskPool.startWorkLoop() ()
#12 0x00005555556d2d94 in core.thread.Thread.run() ()
#13 0x0000555555706788 in thread_entryPoint ()
#14 0x00007ffff7bc108c in start_thread () from /usr/lib/libpthread.so.0
#15 0x00007ffff6f89e7f in clone () from /usr/lib/libc.so.6

Thread 5 (Thread 0x7ffff5690700 (LWP 13236)):
#0  0x00007ffff7bcb260 in nanosleep () from /usr/lib/libpthread.so.0
#1  0x00005555556d2b52 in _D4core6thread6Thread5sleepFNbNiSQBf4time8DurationZv ()
#2  0x0000555555708228 in core.internal.spinlock.SpinLock.yield(ulong) shared ()
#3  0x00005555557081c4 in core.internal.spinlock.SpinLock.lock() shared ()
#4  0x00005555556dc781 in _D2gc4impl12conservativeQw14ConservativeGC__T9runLockedS_DQCeQCeQCcQCnQBs12mallocNoSyncMFNbmkKmxC8TypeInfoZPvS_DQEgQEgQEeQEp10mallocTimelS_DQFiQFiQFgQFr10numMallocslTmTkTmTxQCzZQFcMFNbKmKkKmKxQDsZQDl ()
#5  0x00005555556d60b2 in _D2gc4impl12conservativeQw14ConservativeGC6mallocMFNbmkxC8TypeInfoZPv ()
#6  0x00005555556ad61f in gc_malloc ()
#7  0x00005555556af3f9 in _d_allocmemory ()
#8  0x000055555565aabc in dspec_tester.main(immutable(char)[][]).__foreachbody3(std.file.DirEntry) (
    this=0x7fffffffd450, file=...) at dspec_tester.d:125
#9  0x0000555555664e13 in _D3std11parallelism__T15ParallelForeachTSQBn9algorithm9iteration__T12FilterResultS_DQDe4file10dirEntriesFAyaQdEQEfQBb8SpanModebZ1fMFNaNbNfSQFhQCd8DirEntryZbTSQGaQCw11DirIteratorZQEhZQGe7opApplyMFMDFQCiZiZ4doItMFZv (this=0x7fffffffd290)
    at /home/seb/dlang/phobos/std/parallelism.d-mixin-4011:4183
#10 0x00005555556b995c in _D3std11parallelism__T3runTDFZvZQkFQiZv ()
#11 0x00005555556b9498 in _D3std11parallelism__T4TaskSQBaQz3runTDFZvZQv4implFPvZv ()
#12 0x00005555556eef67 in std.parallelism.AbstractTask.job() ()
#13 0x00005555556b808c in _D3std11parallelism8TaskPool5doJobMFPSQBkQBj12AbstractTaskZv ()
#14 0x00005555556b81ea in std.parallelism.TaskPool.executeWorkLoop() ()
#15 0x00005555556b8191 in std.parallelism.TaskPool.startWorkLoop() ()

Thread 1 (Thread 0x7ffff7f9ca80 (LWP 13228)):
#0  0x00007ffff7bc9cf6 in do_futex_wait.constprop () from /usr/lib/libpthread.so.0
#1  0x00007ffff7bc9de8 in __new_sem_wait_slow.constprop.0 () from /usr/lib/libpthread.so.0
#2  0x0000555555706f26 in thread_suspendAll ()
#3  0x00005555556da3db in _D2gc4impl12conservativeQw3Gcx11fullcollectMFNbbZm ()
#4  0x00005555556d8b7e in _D2gc4impl12conservativeQw3Gcx8bigAllocMFNbmKmkxC8TypeInfoZPv ()
#5  0x00005555556dc7d9 in _D2gc4impl12conservativeQw14ConservativeGC__T9runLockedS_DQCeQCeQCcQCnQBs12mallocNoSyncMFNbmkKmxC8TypeInfoZPvS_DQEgQEgQEeQEp10mallocTimelS_DQFiQFiQFgQFr10numMallocslTmTkTmTxQCzZQFcMFNbKmKkKmKxQDsZQDl ()
#6  0x00005555556d60b2 in _D2gc4impl12conservativeQw14ConservativeGC6mallocMFNbmkxC8TypeInfoZPv ()
#7  0x00005555556ad61f in gc_malloc ()
#8  0x00005555556aca94 in core.memory.GC.malloc(ulong, uint, const(TypeInfo)) ()
#9  0x00005555556e472f in _D3std5array__T14arrayAllocImplVbi0TAhTmZQBaFNaNbmZQp ()
#10 0x00005555556e46d1 in _D3std5array__T18uninitializedArrayTAhTymZQBbFNaNbNeymZQt ()
#11 0x00005555556b53d3 in std.file.readImpl(const(char)[], const(char)*, ulong) ()
#12 0x0000555555665c9a in _D3std4file__T4readTAyaZQkFNfQjmZAv (upTo=18446744073709551615, name=...)
    at /home/seb/dlang/phobos/std/file.d:300
#13 0x0000555555665bb8 in _D3std4file__T8readTextTAyaTQeZQrFNfQmZQp (name=...)
    at /home/seb/dlang/phobos/std/file.d:491
#14 0x0000555555665b90 in _D3std4file__T8readTextTAyaTSQBbQBa8DirEntryZQBfFNfKQyZQBf (name=...)
    at /home/seb/dlang/phobos/std/file.d:510
#15 0x0000555555665b14 in _D12dspec_tester4mainFAAyaZ__T9__lambda2TS3std4file8DirEntryTQBmZQBjFNfQBeQBzZSQBl9algorithm9iteration__T9MapResultSQEk15ddocMacroToCodeTSQDsQChQCa__TQBtSQFw23untilClosingParenthesesTSQFmQEbQDu__T8splitterVQHba6_61203d3d2062TQHuTQHyZQBlFQIgQIjZ6ResultZQFrZQFv (__HID10=0x7fffffffbad0, 
    ddocKey=..., file=...) at dspec_tester.d:115
``

I'm not entirely sure what's going on. Is the GC trying to collect the `uninitializedArray` (?), but it's definitely an issue with the spec tester and not your fault. Sorry for asking you to try an experimental feature.

Copy link
Contributor Author

@timotheecour timotheecour Feb 13, 2018

Choose a reason for hiding this comment

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

side question: why is the backtrace only partially demangled even though:

 echo _D3std5array__T14arrayAllocImplVbi0TAhTmZQBaFNaNbmZQp|ddemangle
pure nothrow ubyte[] std.array.arrayAllocImpl!(false, ubyte[], ulong).arrayAllocImpl(ulong)

I think std.file.read allocates and causes a GC collection, but what happens then i don't know (guess your backtrace doesn't show all threads)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timotheecour timotheecour force-pushed the pr_spec_variadic_default branch from 277c1ad to 2ff15a7 Compare February 22, 2018 10:45
@timotheecour
Copy link
Contributor Author

PTAL, changed back from SPEC_RUNNABLE_EXAMPLE_RUN to SPEC_RUNNABLE_EXAMPLE_COMPILE to circumvent https://issues.dlang.org/show_bug.cgi?id=18490

@wilzbach wilzbach force-pushed the pr_spec_variadic_default branch from 2ff15a7 to 32faefe Compare February 23, 2018 17:30
Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

FYI: I just squashed the fixup commits and we should try to get this in before 2.079 is released ...

spec/template.dd Outdated
return T.length;
}
assert(fun(1, "foo") == 2); // not 1 (IFTI)
assert(fun!int(1, "foo") == 1); // IFTI
Copy link
Member

Choose a reason for hiding this comment

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

Hmm the comments here don't make much sense anymore. I suggest we either remove them or use a better explanation. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(updated, after discussed on slack)

@timotheecour timotheecour force-pushed the pr_spec_variadic_default branch from 50b10a8 to 557f7f8 Compare February 23, 2018 18:38
Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Thanks!

@dlang-bot dlang-bot merged commit 3908cc9 into dlang:master Feb 24, 2018
@timotheecour timotheecour deleted the pr_spec_variadic_default branch February 24, 2018 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants