add astenums.d, removing over 230 lines of redundant code

[WalterBright]

It's an endless source of irritation and inconsistencies to have several enums re-defined in multiple files. This pulls them out into a new file, astenums.d

[dlang-bot]

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 run digger -- build "master + dmd#12744"

[thewilsonator]

There seem to be a bunch of unrelated changes in addition to the extraction of enums.

Apart from the ASTBase duplications (cc @RazvanN7 ) this doesn't seem to be that much of a problem and do we really need more files in src/dmd?

[WalterBright]

There seem to be a bunch of unrelated changes in addition to the extraction of enums.

I know. I borked up my commits. Should be fixed now.

Apart from the ASTBase duplications (cc @RazvanN7 ) this doesn't seem to be that much of a problem and do we really need more files in src/dmd?

We need more files more than we need duplication of enums.

[WalterBright]

this doesn't seem to be that much of a problem

It is a problem because this PR uncovered numerous inconsistencies.

[ibuclaw]

astbase.d in general is an endless source of irritation, but that's another matter.

[WalterBright]

Every new declaration has to be added 4 times:

1. where it belongs
2. in astbase
3. in some .h file
4. in frontend.h

Naturally, error creeps in at every step.

[thewilsonator]

This changes it to

1. in astenums
2. in astbase
3. in some .h file
4. in frontend.h

I don't see how that is much of an improvement.

[WalterBright]

I don't see how that is much of an improvement.

It is removed from astbase.d. 4 copies goes to 3.

[Geod24]

There's an unrelated importC commit in there.

[WalterBright]

I know. I don't know how to delete it. But there are no changes from it.

[WalterBright]

I figured it didn't matter anyway because off squash&merge.

[Geod24]

I figured it didn't matter anyway because off squash&merge.

This requires the reviewer to do the right thing (and care about it) tho.

I know. I don't know how to delete it. But there are no changes from it.

That's not really true. Someone using git blame would be thrown off by seeing this commit, not realizing that it's actually reverted in the next, unrelated commit (if squash isn't used).

When dealing with commit, git rebase -i is a powerful tool.
In this case, with the branch checked out, you can do git rebase -i HEAD~2. It will open your editor with two lines:

pick 64efc10a08 ImportC: add separate tag name symbol table
pick 755e156270 add astenums.d

If you did this originally, I would have recommended to just remove the first line, save and quit. This would drop the commit.
But since now you have a revert of 64efc10a08 in 755e156270, you have to actually squash them locally.
To do this, change the above to:

pick 64efc10a08 ImportC: add separate tag name symbol table
s 755e156270 add astenums.d

Save & quit. Your editor will re-open, and you can edit the commit message of the commit.
By default, it will be the concatenation of both commits, so in this case you just want to remove the "ImportC" part and keep "add astenums.d".

EDIT: Word of caution: If you try to do interactive rebase of a commit that includes a merge commit, it will mess things up.
In the case that happens, two solutions: If you are still in the editor, delete everything, and it will abort the process.
If you are in the middle of interactively rebasing, you can always use git rebase --abort.

[WalterBright]

@Geod24 thanks for the decent instructions. I didn't know rebase had an interactive mode. I closed my eyes and did it, and it worked :-)

[dkorpel]

Can you add the new file to the README.md in src/dmd?

[ibuclaw]

Before we do this, astbase should really be fixed first to remove all inconsistencies. Because they only highlight that astbase is wrong, not the dmd front-end code.

[PetarKirov]

@WalterBright this PR is an excellent example why one commit per PR is a bad rule to follow. (Yet, I agree that PRs shouldn't mix refactorings and bug fixes if possible.) In a single commit you have modified 66 (!) files which makes it quite difficult to review. Instead this PR should include a series of commits, each of which moving a single enum to astenums.d and updating only the set of affected files. When a big change is broken in a series of independent (if possible orthogonal) changes it is much easy to verify the correctness. At the same time you don't need to open 20 different PRs and if there's a bug in one of the commits only that commit can be reverted, instead of reverting the whole series. Also, once you have the change split into multiple commits, you could easily split it across multiple PRs as well.

Step 1 (current state):

<one big commit>
                \
            your branch
                 |
                 v
          This pull request

Step 2 - split the big commit into multiple smaller ones:

<small commit 1> <- <small commit 2> <- <small commit 3> <- <small commit 4> <- <small commit 5>
                                                                                                \
                                                                                            your branch
                                                                                                 |
                                                                                                 v
                                                                                          This pull request

Step 3 (optional) - you can branch at any previous commit if you want to create separate PRs:

<small commit 1> <- <small commit 2> <- <small commit 3> <- <small commit 4> <- <small commit 5>
                                    \                  \                                        \
                                  branch 1           branch 2                                 branch 3
                                     |                  |                                        |
                                     v                  v                                        v
                               Pull request 1     Pull request 2                           Pull request 3

This is not about personal preferences, but about an established workflow of using Git that has proven itself extremely useful over the years across the whole industry that saves time for both the developers and the reviewers, while increasing the quality of PRs.

If you're unsure how to do that, I, @Geod24, @ibuclaw @CyberShadow, @MoonlightSentinel and many others will be more than happy to help you!

[WalterBright]

this PR is an excellent example why one commit per PR is a bad rule to follow.

This PR is trivial to review. It's just cut&paste. Yes it modifies 66 files, including the one you asked me to modify. Nearly all of them are simply adding an import declaration. There is nothing hard about this review.

But reasonable people can disagree. If anyone wants to take this PR over and spend the time altering it, I'll approve it.

Addressed

[thewilsonator]

@RazvanN7 this didn't address the adding even still more files.

@WalterBright Well I guess I'm just going to start packaging wether you like it or not.

[WalterBright]

this didn't address the adding even still more files.

I addressed it: #12744 (comment)

[thewilsonator]

This still adds more files, I don't see how you could have possibly addressed that.