Add ddoc for -i #7777

marler8997 · 2018-01-25T17:16:35Z

No description provided.

dlang-bot · 2018-01-25T17:16:36Z

Thanks for your pull request and interest in making D better, @marler8997! 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.

wilzbach · 2018-01-25T17:23:28Z

src/dmd/cli.d

        Option("i=[-]<pattern>,[-]<pattern>,...",
-            "include/exclude imported modules whose name matches one of <pattern>"
+            "include/exclude imported modules whose name matches one of <pattern>",
+            `this option enables "include import" mode, where the compiler will


While the dlang.org script will capitalize the first word automatically, I think it's still good to do here too (to avoid confusion etc.).

wilzbach · 2018-01-25T17:23:49Z

src/dmd/cli.d

+
+             Determining which modules to include is based on a set of "module patterns".
+             These patterns are optionally included after the "-i=" option as a comma
+             separated list.  A pattern matches any module whose fully qualified name


Double whitespace

wilzbach · 2018-01-25T17:24:09Z

src/dmd/cli.d

+             "foo.bar" would not match a module named "foo.barx".
+
+             The presence of the "-" prefix in a module pattern indicates that matching-modules
+             should be excluded from compilation.  Patterns without this indicate that


wilzbach · 2018-01-25T17:25:04Z

src/dmd/cli.d

+             want to override this behavior, they can use the special "." pattern to "include by
+             default" or "-." to "exclude by default".
+
+             Note: multiple "-i=..." options is allowed, each one just adds more patterns.`


Avoid "just" as it colloquial.

wilzbach · 2018-01-25T17:27:52Z

src/dmd/cli.d

+             on the command line.
+
+             Determining which modules to include is based on a set of "module patterns".
+             These patterns are optionally included after the "-i=" option as a comma


How about using a delimited string and use back-ticks for command line things like -i=?

wilzbach · 2018-01-25T17:28:46Z

src/dmd/cli.d

+             Note that along with the module patterns given by the user, there is a standard
+             set of exclusionary module patterns that are always used, namely:
+                "-std,-core,-etc,-object"
+             Note that these can be overriden (i.e. -i=std,core,etc,object).


Double "Note", maybe "However" ?

wilzbach · 2018-01-25T17:30:49Z

src/dmd/cli.d

+             Note that these can be overriden (i.e. -i=std,core,etc,object).
+
+             Since module patterns can either be inclusionary or exclusionary, a priority
+             must be established to handle the case where a module matches multiple patterns.


I think typically the motivation for a feature isn't stated in its CLI docs.
@ others agree or disagree?

wilzbach · 2018-01-25T17:34:19Z

src/dmd/cli.d

+             should be excluded from compilation.  Patterns without this indicate that
+             matching-modules should be included in compilation.
+
+             Note that along with the module patterns given by the user, there is a standard


The style of addressing the user as "the user" is completely new and doesn't appear anywhere at https://dlang.org/dmd.html

I think sth. like this would fit better with the existing style:

The standard set of exclusionary module patterns is "-std,-core,-etc,-object" and can be overridden with i.e. -i=std,core,etc,object).

wilzbach · 2018-01-25T17:40:03Z

src/dmd/cli.d

+             by default.  However, if at least one "inclusionary" module pattern is given, then
+             modules that don't match any pattern will be excluded by default.  Should the user
+             want to override this behavior, they can use the special "." pattern to "include by
+             default" or "-." to "exclude by default".


This paragraph is a bit too complicated for what it tries to tell. How about:

By default modules that don't match any pattern will be included.
However, if at least one inclusionary module pattern is given, then modules that don't match any pattern will be excluded.
This can be overridden by -i=-. (exclude by default) or -i=. (include by default).

wilzbach · 2018-01-25T17:45:32Z

Oh and congrats to submitting 7777 🍀 🥇

timotheecour · 2018-01-25T19:10:34Z

src/dmd/cli.d

-            "include/exclude imported modules whose name matches one of <pattern>"
+            "include/exclude imported modules whose name matches one of <pattern>",
+            q"{$(P Enables "include imports" mode, where the compiler will include imported
+	     modules in the compilation, as if they were given on the command line.)


as if they were given on the command line

there seems to be subtle differences: are these a feature or a bug? should they be documented here?

main.d: import foo.fun; foo/fun.d:

dmd -i -o- main.d
#ok
dmd -o- main.d foo/fun.d
Error: module fun from file foo/fun.d must be imported with 'import fun;'

NOTE: if we add module foo.fun; to foo/fun.d the problem goes away
NOTE: if we add module asdf.fun; to foo/fun.d, dmd -i accepts the code but should not. This one is a clear bug.

Fix here: #7778

timotheecour · 2018-01-25T19:29:39Z

src/dmd/cli.d

+            "include/exclude imported modules whose name matches one of <pattern>",
+            q"{$(P Enables "include imports" mode, where the compiler will include imported
+	     modules in the compilation, as if they were given on the command line.)
+


another subtle difference, again not clear if feature or bug but should be documented: behavior is different with -op with and without -i

dmd -od=generated -op -c -Ifoo foo/fun1.d foo/bar/fun4.d foo/main.d dmd -od=generated_i -op -c -i -Ifoo foo/main.d

foo/main.d: void main(){import bar.fun4; import fun1;} foo/fun1.d: foo/bar/fun4.d:module bar.fun4;

├── generated │ └── foo │ ├── bar │ │ └── fun4.o │ ├── fun1.o │ └── main.o ├── generated_i │ ├── bar │ │ └── fun4.o │ ├── foo │ │ └── main.o │ └── fun1.o

yeah looks like a bug, I"ll see if I can fix this one

timotheecour

bugs or features? need doc

wilzbach

There is still a bit of an inconsistency between backticks and quotes which I assume to look a bit weird on the rendered docs, apart from that it looks really good. Thanks!

wilzbach · 2018-01-28T11:00:51Z

src/dmd/cli.d

+             These patterns are optionally included after `-i=` as a comma separated list.
+             A pattern matches any module whose fully qualified name starts with the pattern.
+             The pattern "foo.bar" matches all modules in the `foo.bar` package, such as
+             "foo.bar" itself, "foo.bar.baz" or even "foo.bar.package". Note that each component


As you escape package names below, I would do it here too for the sake of consistency.

I was attempting to escape package names, but not module patterns...there are other places in the help where module patterns are quoted, but I thought that package names themselves should definitely be escaped. looking at the final generated docs will likely indicate which method looks the best.

wilzbach · 2018-01-28T11:02:51Z

src/dmd/cli.d

+
+             $(P The presence of the "-" prefix in a module pattern indicates that matching-modules
+             should be excluded from compilation. Patterns without this indicate that
+             matching-modules should be included in compilation.)


Why so you use a hyphen here?

wilzbach · 2018-01-28T11:04:09Z

src/dmd/cli.d

+             $(P Along with the module patterns supplied, there is a standard set of exclusionary
+             patterns that are always included:
+                "-std,-core,-etc,-object"
+             Note that these can be overriden (i.e. -i=std,core,etc,object).)


For consistency: use backticks

wilzbach · 2018-01-28T11:04:51Z

src/dmd/cli.d

+
+             $(P Along with the module patterns supplied, there is a standard set of exclusionary
+             patterns that are always included:
+                "-std,-core,-etc,-object"


You use backticks for CLI arguments above.

JinShil · 2018-02-01T04:04:43Z

@wilzbach Why isn't this showing up in DAutoTest's rendered docs?

marler8997 · 2018-02-01T04:58:39Z

I think we're waiting on this: dlang/dlang.org#2068

wilzbach · 2018-02-07T17:09:43Z

As dlang/dlang.org#2068 is in, I rebased this PR to restart DAutoTest.

marler8997 · 2018-02-07T19:02:37Z

Hmmm, the -i=[-]<pattern>,[-]<pattern,.. doesn't seem to show up correctly. Maybe commas are messing it up?

marler8997 · 2018-02-07T20:19:08Z

Yeah I think it's the comma. I think the dlang.org generator needs to be modified to handle commas in the flag field.

marler8997 · 2018-02-08T05:13:00Z

Looks like dlang/dlang.org#2188 did the trick. This PR should be ready for merge.

wilzbach · 2018-02-08T19:57:32Z

src/dmd/cli.d

+             $(P Along with the module patterns supplied, there is a standard set of exclusionary
+             patterns that are always included:
+                `-std,-core,-etc,-object`
+             Note that these can be overriden (i.e. `-i=std,core,etc,object`).)


I think this needs a new line or at least a dot:

Also you use "Note:" later (with colon) - consistency and all ;-)

wilzbach

LGTM module nits.

marler8997 · 2018-02-09T18:37:42Z

Wait for #7857 before merging this one. If that one gets accepted then it will change this one a bit.

marler8997 · 2018-02-13T01:56:40Z

Walter has merged #7857 so I've reworded this one a bit...

wilzbach · 2018-02-13T02:01:27Z

src/dmd/cli.d

+             be excluded. This behavior can be overriden by usig `-i=.` to include by default or `-i=-.` to
+             exclude by default.)
+
+             $(P Note that multiple `-i=...` options are allowed, each one adds more patterns.)}"


How about: each one adds a pattern?

Though, I think now you can even remove this last paragraph completely as you have a few examples of multiple arguments in the paragraphs before.

wilzbach · 2018-02-13T02:01:47Z

src/dmd/cli.d

+             $(P The default behavior of excluding druntime/phobos is accomplished by internally adding a
+             set of standard exclusions, namely, `-i=-std -i=-core -i=-etc -i=-object`. Note that these
+             can be overriden with `-i=std -i=core -i=etc -i=object`.)
+             


file 'dmd/cli.d' contains trailing whitespace at line 329

JinShil · 2018-02-14T04:12:08Z

It looks like this is causing the semaphore failure:

../dmd/generated/linux/release/32/dmd -conf= -I../druntime/import -w -de -dip25 -m32 -fPIC -transition=complex -O -release -unittest -version=StdUnittest -c -ofgenerated/linux/release/32/unittest/std/base64.o std/base64.d
src/rt/tracegc.d(43): Deprecation: The delete keyword has been deprecated. Use object.destroy() instead.

I don't understand why this wasn't caught by the test suite in #7342.

Furthermore, that block of code is in a version(none) block: https://github.com/dlang/druntime/blob/dd5fe6ff8398e7586bc005c3f4986834f211784e/src/rt/tracegc.d#L22-L84

I don't understand.

marler8997 · 2018-02-14T04:15:21Z

Are you sure that's the cause of the failure? It looks like it was a unit test failure in std.datetime.

****** FAIL release64 std.datetime.stopwatch
core.exception.AssertError@std/datetime/stopwatch.d(452): unittest failure
----------------
??:? _d_unittestp [0x20b672a1]
??:? nothrow @safe void std.datetime.stopwatch.__unittest_L442_C15() [0x205bfe83]
??:? void std.datetime.stopwatch.__modtest() [0x205c0314]
??:? [0x4014ab]
??:? [0x4013e3]
??:? [0x4012bb]
??:? runModuleUnitTests [0x20b67ddd]
??:? void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).runAll() [0x20b85558]
??:? void rt.dmain2._d_run_main(int, char**, extern (C) int function(char[][])*).tryExec(scope void delegate()) [0x20b854e3]
??:? _d_run_main [0x20b8544e]
??:? [0x4017cb]
??:? __libc_start_main [0x217bef44]
make[1]: *** [unittest/std/datetime/stopwatch.run] Error 1
make[1]: *** Waiting for unfinished jobs....
0.203s PASS release64 std.datetime
stderr test, please ignore
6.569s PASS release64 std.process
3.202s PASS release64 std.datetime.systime
make[1]: Leaving directory `/home/runner/phobos'
make: *** [unittest-release] Error 2
make: Leaving directory `/home/runner/phobos'

JinShil · 2018-02-14T04:21:15Z

Are you sure that's the cause of the failure?

Sorry, maybe my mistake. I was looking at the ~~GDC~~ DMD x86 log. But I see the std.datetime error in the DMD x86_64 log. Probably best to fix the std.datetime problem first and see what happens.

wilzbach · 2018-02-19T16:13:13Z

changelog/includeimports.dd

 The option -i by itself is equivalent to:

-$(CONSOLE dmd -i=-std,-core,-etc,-object)
+$(CONSOLE dmd -i=-std -i=-core -i=-etc -i=-object)


We probably need to cherry-pick this to stable.

marler8997 mentioned this pull request Jan 25, 2018

Document the new -i dmd compiler option dlang/dlang.org#2109

Closed

wilzbach reviewed Jan 25, 2018

View reviewed changes

marler8997 force-pushed the documentDashI branch from f3e8ee0 to b241573 Compare January 25, 2018 18:06

timotheecour reviewed Jan 25, 2018

View reviewed changes

marler8997 mentioned this pull request Jan 25, 2018

Deprecate import/module name mismatch (ALL CASES) #7778

Closed

marler8997 force-pushed the documentDashI branch 2 times, most recently from 5c80d51 to 71fe41e Compare January 25, 2018 22:53

wilzbach added the Review:Easy Review label Jan 26, 2018

wilzbach reviewed Jan 28, 2018

View reviewed changes

marler8997 force-pushed the documentDashI branch from 71fe41e to 606eb8c Compare January 28, 2018 11:20

wilzbach force-pushed the documentDashI branch from 606eb8c to a7461f5 Compare February 7, 2018 17:09

marler8997 mentioned this pull request Feb 7, 2018

Escape commas in option flag arguments dlang/dlang.org#2188

Merged

marler8997 force-pushed the documentDashI branch from a7461f5 to 90c4b8a Compare February 7, 2018 23:29

JinShil approved these changes Feb 8, 2018

View reviewed changes

wilzbach added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. and removed Merge:72h no objection -> merge The PR will be merged if there are no objections raised. labels Feb 8, 2018

wilzbach reviewed Feb 8, 2018

View reviewed changes

wilzbach approved these changes Feb 8, 2018

View reviewed changes

marler8997 force-pushed the documentDashI branch 2 times, most recently from e960b7a to baf1586 Compare February 8, 2018 20:24

marler8997 force-pushed the documentDashI branch from baf1586 to 874361b Compare February 13, 2018 01:56

wilzbach reviewed Feb 13, 2018

View reviewed changes

marler8997 force-pushed the documentDashI branch 3 times, most recently from faf1e78 to 3bf4958 Compare February 13, 2018 05:32

wilzbach added the Merge:auto-merge label Feb 19, 2018

wilzbach reviewed Feb 19, 2018

View reviewed changes

Add ddoc for -i

988c7ce

marler8997 force-pushed the documentDashI branch from 3bf4958 to 988c7ce Compare February 19, 2018 16:36

dlang-bot removed the Merge:auto-merge label Feb 19, 2018

marler8997 changed the base branch from master to stable February 19, 2018 16:37

JinShil added the Merge:auto-merge label Feb 20, 2018

dlang-bot merged commit b97d2db into dlang:stable Feb 20, 2018

Uh oh!

Add ddoc for -i #7777

Add ddoc for -i #7777

Uh oh!

Conversation

marler8997 commented Jan 25, 2018

Uh oh!

dlang-bot commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheecour Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheecour left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JinShil commented Feb 1, 2018

Uh oh!

marler8997 commented Feb 1, 2018

Uh oh!

wilzbach commented Feb 7, 2018

Uh oh!

marler8997 commented Feb 7, 2018

Uh oh!

marler8997 commented Feb 7, 2018

Uh oh!

marler8997 commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

marler8997 commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dlang-bot commented Jan 25, 2018 •

edited

Loading

timotheecour Jan 25, 2018 •

edited

Loading

marler8997 commented Feb 9, 2018 •

edited

Loading

JinShil commented Feb 14, 2018 •

edited

Loading

JinShil commented Feb 14, 2018 •

edited

Loading