Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Ran dfix on druntime and replaced old alias syntax #1002

Closed
wants to merge 2 commits into from
Closed

Ran dfix on druntime and replaced old alias syntax #1002

wants to merge 2 commits into from

Conversation

joakim-noah
Copy link
Contributor

There were a couple bugs in the dfix output that I had to manually replace, but most of this patch was automatically generated. I then manually checked it, though after a certain point my eyes glazed over on simple type aliasing, ie those without parametrized types or function/delegate pointers, as it was getting them all right.

The alignment on a column is mucked up in some places so that the reading flow isn't as good, will look into getting that whitespace formatting right if this pull is otherwise deemed worthwhile.

@jmdavis
Copy link
Member

jmdavis commented Oct 27, 2014

Honestly, I see this as pointless churn unless we decide to deprecate the older syntax, and that hasn't happened. Others may disagree though.

@jmdavis
Copy link
Member

jmdavis commented Oct 27, 2014

I will say though that it's cool that dfix can do this.

@JinShil
Copy link
Contributor

JinShil commented Oct 27, 2014

I'm very happy to see this PR. When I first started learning D this caused me some frustration, not because I was confused by the two syntaxes, but because I couldn't reconcile which one was the recommended syntax. Even after consulting advice on the forum, all I was able to determine was everyone else was also in limbo. Such things make the D effort look careless and undisciplined, and in some cases can impede progress. They are as much of a liability as a messy workshop.

I personally prefer the new syntax and I'm glad the author made the effort to introduce it. Attention to such details make the language feel more polished, and make programming in D more like drinking a cool lemonade than a warm beer. The old syntax is vague to someone like me that spends their time mentally shifting between multiple languages, and it's arguably inconsistent with the "right-to-left assignment flow" of C-like languages. With the new syntax, I don't have to make a second thought "What was the order again?". I do the same mental second-glance with C typedefs.

Thumbs up! And thanks for tidying up. But, it might be easier to review and certify in chunks. Maybe break it up by folder?

@joakim-noah
Copy link
Contributor Author

Updated this pull with the last couple files that had to be manually reconciled by hand because, for example, dfix couldn't automatically convert this:

 alias CHAR*         LPCH,  LPSTR,  PCH,  PSTR;

I also corrected a documentation comment having to do with Android's TLS implementation, which I realized I had written up incorrectly.

As for the value of this pull, if we're going to recommend this new alias syntax, druntime and phobos should use it everywhere. Given how straightforward it is to update druntime and phobos with dfix, I see no reason to encourage the confusion @JinShil expressed from having both forms all throughout the runtime and standard library, particularly given how cheap it is to fix.

I'm fine with splitting it up into chunks if that's what the reviewers prefer.

@yebblies
Copy link
Member

We have not deprecated the old syntax, both are valid. While I would recommend the new syntax for new code, this patch adds very little value.

@schveiguy
Copy link
Member

I don't see any reason to deprecate the old syntax, it's not ambiguous, even if it is not as descriptive. Note, the only place where I have to ever look up to try and figure out which order to do things is alias this (I couldn't tell you right now what the correct way to do it is). And from what I remember, alias this cannot use the new syntax for some reason.

What happens in ddoc? If we make sure ddoc only shows the new alias syntax, then I don't think any actual code needs to be fixed.

@joakim-noah
Copy link
Contributor Author

Nobody has suggested deprecating the old syntax. If we're going to recommend the new syntax, we should use it exclusively, rather than have druntime/phobos mostly use the old syntax. One argument that could've been made is that it's too much work to update all usage of the old syntax in druntime/phobos: dfix takes away that excuse.

@schveiguy
Copy link
Member

If it's not going to be deprecated, it's valid. I don't know that we need even to recommend either syntax, as there are some that prefer each.

In which case, this PR is not doing much. It's akin to changing indentation, or capitalization of variable names. It's not going to change anyone's code, or break any code. It's also not going to make anything any better. Do we have any official position on which syntax is accepted for library code in our often-ignored style guidelines?

@joakim-noah
Copy link
Contributor Author

Yes, the old syntax is still valid, don't know who prefers it though. I'd say it's akin to moving all function attributes to the right: it's more clear and should be the style used through druntime/phobos, even if the old alias and left-hand attribute syntax is still supported by the compilers for legacy code.

@JinShil
Copy link
Contributor

JinShil commented Oct 27, 2014

I think this pull request may have been submitted prematurely. This pull request is more about coding conventions and style than deprecation of the old syntax. No one has recommended deprecating the old syntax.

Therefore, I invite everyone to consider adopting a recommendation at dlang/dlang.org#682 first. I believe that decision will guide this one.

@schveiguy
Copy link
Member

I'd say it's akin to moving all function attributes to the right

That is a very different problem. Because alias x does not mean different things depending on context, it means "alias the symbol x to something else"

But const T means "const version of T type" in some contexts, but in function attributes means "const function that returns mutable T". Old alias syntax may be somewhat unintuitive, but it's consistent.

@joakim-noah
Copy link
Contributor Author

I was under the impression that the new syntax was already recommended, which Dan alluded to when he said above that it was recommended for new code. I think the new syntax is unambiguously better and more consistent with the rest of the language, but I'm fine with waiting on a decision on @JinShil's doc pull first.

@jmdavis
Copy link
Member

jmdavis commented Oct 27, 2014

Yes, the old syntax is still valid, don't know who prefers it though.

I, for one, much prefer the older syntax and don't like the new one all. I find it to be completely backwards. But regardless of my personal preference, if both syntaxes are legal, then I agree that it's akin to moving all of the function attributes to one side of the function, which is useless churn. I wouldn't be in favor of this pull even if it were changing all of the alias statements so that they used the older syntax, which I prefer.

Sure, many folks prefer the newer syntax, just like many folks prefer using UFCS over more traditional, left-to-right function chaining, but for the most part, that's not a question of technical merit but rather personal preference, and I don't see any reason to be mandating one alias syntax over the other any more than we mandate that UFCS be used or not.

@JinShil
Copy link
Contributor

JinShil commented Oct 28, 2014

I was under the impression that the new syntax was already recommended.

I was too. I'm a bit surprised by the controversy here.

@MartinNowak
Copy link
Member

I'm a bit surprised by the controversy here.

I'm surprised too.
Anyhow I would not want to mass rewrite druntime because it pollutes the git history (breaks blame for example). Let's change this incrementally whenever someone shuffles code around.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants