document DMD style more prominently by WalterBright · Pull Request #9511 · dlang/dmd

WalterBright · 2019-03-29T09:48:05Z

This originally appeared in the n.g.:

https://digitalmars.com/d/archives/digitalmars/D/internals/DMD_Technical_Debt_470.html

Given the many recent refactorings submitted, I thought it best to put this in a more prominent place.

dlang-bot · 2019-03-29T09:48:06Z

Thanks for your pull request, @WalterBright!

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.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9511"

thewilsonator · 2019-03-29T10:28:06Z

CONTRIBUTING.md

+across the source tree is likely to be disruptive and unlikely to provide
+significant improvement.
+
+5. Changes of more than 500 lines need buyoff from Andrei or Walter.


buyoff is not a word did you mean buy-in? Also I don't think this is a good idea.

I think they mean the same thing.

buyoff doesn't appear in my dictionary.

This does not help a lot, either :o). https://www.merriam-webster.com/dictionary/buy%20off

thewilsonator · 2019-03-29T10:32:34Z

CONTRIBUTING.md

+
+2. Shuffling all the code about
+
+3. Creating packages


Ditto. I'd love to reorganise the compiler into ast, sema ...

That's in the negative column :-)

I know, hence "ditto (I don't think this is a good idea.)" . Creating packages is something we should be doing not something we should be avoiding.

src/dmd/ contains ~120 d source files in one directory, with no organisation. This is getting unwieldy. Taking a cursory glance I see at least the following packages:

AST (e.g. aggregate.d)

backend interfacing (e2ir.d, s2ir.d, toir.d) (incidentally the are removed from LDC's copy of the front end)

optimisations (constfold.d, inline[|cost].d)

iasm

lib (lib[|elf|mach|mscoff|omf].d) (so are these)

visitors

scan (scan[elf|mach|mscoff|omf].d) (so are these)

semantic analysis (semantic[2|3].d, typesem.d, statementsem.d, templateparamsem.d

utils (util.d, filecache.d, intrange.d)

We should also get rid of the headers into their own folder (I mean we should get rid of them all together when we can generate them, but we're not there yet).

I agree that we shouldn't over-package the codebase, but we have the opposite problem at the moment. Compare clang with dmd

Been there before: #7639 :/
I don't know anyone who's happy with the status quo (except for Walter, of course).

Well I've put this on the AGM agenda.

Oh...I guess I thought it was originally written for Linux since it's so much easier to develop dmd on Linux.

No. It's just that he has his own way of developing on Windows compared to how everyone else is doing Windows development. Custom compiler, linker, object format and so on. But on Linux he kind of follows what most developers are doing.

Symlinks don't work that well on Windows?

They don't? Wikipedia seems to imply there aren't any major issues. https://en.wikipedia.org/wiki/NTFS_symbolic_link

"The default security settings in Windows Vista/Windows 7 disallow non-elevated administrators and all non-administrators from creating symbolic links. [..]
Starting with Windows 10 Insiders build 14972 the requirement for elevated administrator privileges was removed, allowing symlinks to be created without needing to elevate the console as administrator [1].

[..]
Windows Installer does not fully support symbolic links. A redirected \Windows\Installer will cause most .msi-based Windows installers to fail with error 2755 and/or error 1632."

In general symlinks / junction points are a can of worms on Windows. Mostly because people expect them to work just like on Posix systems, but that's far from the case.

Just as single data point, Git for Windows' support for symlinks is disabled by default.

If we're serious about this (I was only half serious) then it only needs to work on Walter's box :)

jacob-carlborg · 2019-03-29T11:00:58Z

CONTRIBUTING.md

+This was done successfully in src/dmd/escape.d, it wasn't easy, but
+it was well worth it.
+
+17. Try to eliminate reliance on `global.errors`.


Can we change this to global, or is it too drastic?

There's a particular problem with global.errors

wilzbach · 2019-03-29T12:27:34Z

CONTRIBUTING.md

+
+21. The more constrained the scope of a name is, the shorter it should be.
+
+


Can we add: follow the DStyle for new code?

It already addresses just sticking with the existing style of the code.

The "existing" style isn't defined and varies from file to file and sometimes even function to function.

A link to the description of the style would be good. Assuming that you mean the style as documented on dlang.

Yup I'm talking about https://dlang.org/dstyle
(the one that we require for all the other core D repositories).

JinShil · 2019-04-15T03:45:13Z

CONTRIBUTING.md

+
+5. Changes of more than 500 lines need buy-in from Andrei or Walter.
+
+6. Reformatting into your personal style. Please stick with the existing style.


No. Use the The D Style

JinShil · 2019-04-15T03:46:29Z

CONTRIBUTING.md

+
+3. Creating packages
+
+4. As a general rule, any improvement that is implemented by using sed scripts


That depends entirely on what the change is, not how, or what tool was used to make the change.

That's why it says "as a general rule" not "as an inviolate rule".

JinShil · 2019-04-15T03:47:41Z

CONTRIBUTING.md

+
+## The following will not be viewed with favor:
+
+1. Grand renamings of variables and functions


If it solves an inconsistency issue or makes the code more idiomatic-D, it could be quite beneficial.

I'll put it another way. Changes that go against the recommendations here need my approval.

We have The D Style, but DMD doesn't follow it. That's a problem, IMO. Why can't we do something about it and bring Phobos, druntime and DMD all under the same style? You are always welcome to make a PR against The D Style document to improve it if there's something you don't like about it. Furthermore, if DMD were updated to be consistent with The D Style, we could enable DScanner linting checks to enforce that it stays that way now and into the future.

I don't have any problem with the D Style. I'm afraid, though, of opening the floodgates to massive PRs to re-style the code, which is an Illusion of Progress. It won't remove one iota of technical debt, but it'll sure be disruptive and waste everyone's time.

Yes, it will open up the floodgates to massive PRs temporarily. Let's get it over and done with.

@wilzbach cool, that stuff should be at least enabled for new PRs.

No, can't do. It would then lead to new PRs needing to make those unwanted "unrelated changes".
It needs to be enabled separately (either for the entire project or specific modules).

I disagree with this.

We have very different view of that "useful" is. I'm going to stop replying now since this is completely pointless and doesn't lead anywhere.

We have very different view of that "useful" is.

That creates opportunity.

@wilzbach I understand. Style adjustments for uniformity have usefulness because they eliminate certain debate going forward, and are enforceable automatically. Please send such my way (starting with the most useful/least disruptive) and I'll have them in. Thanks.

Sorry, I gave up on these. I have more important fights to fight these days.

JinShil · 2019-04-15T03:48:02Z

CONTRIBUTING.md

+
+20. The function return value variable should be named `result`.
+
+21. The more constrained the scope of a name is, the shorter it should be.


JinShil · 2019-04-15T03:48:10Z

CONTRIBUTING.md

+
+Such functions should not be mutating the data nor issuing error messages.
+
+20. The function return value variable should be named `result`.


JinShil · 2019-04-15T03:57:53Z

CONTRIBUTING.md

+
+3. Do not use `strlen`/`strcmp` and their ilk. Use D arrays instead.
+
+4. Use `ref` instead of raw pointers.


Seems like a good idea, but I'd like to know more about when to use ref and when to use pointers. I would like to learn more.

ref should pretty much always be used where it can be used instead of a pointer.

Unless it's const ref, I avoid ref because it means the caller can't tell that its value could be modified.

int x = 42; foo(x); // what is x?

Since I avoid non-const ref, I know that x is still 42. I don't have to worry about the immediate value of my parameters changing when I pass them to functions.

C# also supports ref, but requires that the caller specify it as well:

int x = 42; foo(x); // x is still 42 bar(ref x); // now we know x may have been modified

This is a big disadvantage for non-const ref in my book as the immediate value of every single argument I pass to a function can now change from underneath me. It adds a very large mental burden on the developer.

JinShil · 2019-04-15T03:58:04Z

CONTRIBUTING.md

+are often overkill for nested functions and function literals; use ordinary
+comments for those.
+
+3. Do not use `strlen`/`strcmp` and their ilk. Use D arrays instead.


JinShil · 2019-04-15T03:59:02Z

CONTRIBUTING.md

+1. Use attributes `const`/`nothrow`/`pure`/`scope`/`@safe`/`private`/etc.
+Successfully using `pure` functions is regarded with particular favor.
+
+2. Use correct Ddoc function comment blocks. Do not use Ddoc comments for


Good! And please follow The D Style for comment blocks.

JinShil · 2019-04-15T03:59:36Z

CONTRIBUTING.md

+we expect new code to, and PRs retrofitting the existing code to
+follow it is welcome.
+
+1. Use attributes `const`/`nothrow`/`pure`/`scope`/`@safe`/`private`/etc.


JinShil · 2019-04-15T04:00:41Z

CONTRIBUTING.md


 If you want a hassle-free contribution look for issues categorized as [preapproved](https://issues.dlang.org/buglist.cgi?component=dmd&keywords=preapproved&product=D).

+## DMD Style


The text in this section seems to be less about style and more about best practices and guidelines.

JinShil · 2019-04-15T04:11:20Z

Recommendation. Create a separate PR for this without the controversial items, and leave the controversial items for a followup PR.

thewilsonator · 2019-05-22T05:55:59Z

Recommendation. Create a separate PR for this without the controversial items, and leave the controversial items for a followup PR.

#9833

thewilsonator · 2019-05-25T07:48:30Z

Please rebase/reopen if you intend to continue the discussion.

WalterBright added the Severity:documentation label Mar 29, 2019

thewilsonator reviewed Mar 29, 2019

View reviewed changes

jacob-carlborg reviewed Mar 29, 2019

View reviewed changes

wilzbach reviewed Mar 29, 2019

View reviewed changes

document DMD style more prominently

7c161aa

WalterBright force-pushed the techdebt branch from 387dce4 to 7c161aa Compare April 14, 2019 10:07

JinShil suggested changes Apr 15, 2019

View reviewed changes

WalterBright mentioned this pull request May 2, 2019

Refactoring: Make dmd.root.file.File stateless #9728

Merged

thewilsonator mentioned this pull request May 22, 2019

document DMD style more prominently #9833

Merged

thewilsonator mentioned this pull request May 23, 2019

Make lib a package #9844

Closed

thewilsonator closed this May 25, 2019


		21. The more constrained the scope of a name is, the shorter it should be.


		5. Changes of more than 500 lines need buy-in from Andrei or Walter.

		6. Reformatting into your personal style. Please stick with the existing style.


		3. Creating packages

		4. As a general rule, any improvement that is implemented by using sed scripts


		## The following will not be viewed with favor:

		1. Grand renamings of variables and functions


		20. The function return value variable should be named `result`.

		21. The more constrained the scope of a name is, the shorter it should be.


		Such functions should not be mutating the data nor issuing error messages.

		20. The function return value variable should be named `result`.


		3. Do not use `strlen`/`strcmp` and their ilk. Use D arrays instead.

		4. Use `ref` instead of raw pointers.


		If you want a hassle-free contribution look for issues categorized as [preapproved](https://issues.dlang.org/buglist.cgi?component=dmd&keywords=preapproved&product=D).

		## DMD Style

Uh oh!

Conversation

WalterBright commented Mar 29, 2019

Uh oh!

dlang-bot commented Mar 29, 2019

Bugzilla references

Testing this PR locally

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!

thewilsonator Mar 30, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

marler8997 May 31, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

PetarKirov Jun 1, 2019 • 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!

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 Mar 30, 2019 • edited by PetarKirov 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!

Choose a reason for hiding this comment

Uh oh!

WalterBright Apr 19, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

JinShil Apr 19, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

thewilsonator Mar 30, 2019 •

edited

Loading

marler8997 May 31, 2019 •

edited

Loading

PetarKirov Jun 1, 2019 •

edited

Loading

wilzbach Mar 30, 2019 •

edited by PetarKirov

Loading

WalterBright Apr 19, 2019 •

edited

Loading

JinShil Apr 19, 2019 •

edited

Loading

JinShil Apr 15, 2019 •

edited

Loading

marler8997 Jun 1, 2019 •

edited

Loading