Refactor pragma semantic

[thewilsonator]

cc @ibuclaw @JinShil

[dlang-bot]

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

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#8557"

[ibuclaw]

Nice, but needs some more work to make it usable by compilers implementing their own vendor pragmas.

[ibuclaw]

Also, you should reference three other issues here. 3004, 19109, and 19110.

[WalterBright]

Please do NOT combine large refactorings with bug fixes.

[WalterBright]

Please do the bug fix and the refactorings as separate PRs, because 1. that makes them infinitely easier to review and 2. PRs should be one issue per PR.

[JinShil]

@thewilsonator Do you plan on heeding @WalterBright's advise here and split the refactor out into it's own PR?

[thewilsonator]

Sorry I've been sick all week and probably most of next week. I will, but If you want to get it done faster, I won't stop you from doing it.

[JinShil]

No worries. I'd rather you did it; you might need me to merge it, and I can't merge my own PRs.

[thewilsonator]

This is just the refactoring, no target specific handling, no bug fixing.

[thewilsonator]

Oh wonderful, Im corrupting something somewhere and object file emission segfaults. I guess this will have to wait until I'm not sick.

[andralex]

This kind of work needs to stop. There's no discernible purpose to this refactoring and no discernible improvement in the code following. It actually adds code!

[thewilsonator]

On the contrary, this centralises pragma semantic to one file and make it much easier to be consistent between PragmaDeclarations and PragmaStatements. See
https://issues.dlang.org/show_bug.cgi?id=19149 https://issues.dlang.org/show_bug.cgi?id=19109

[JinShil]

@andralex, expressionsem.d is 11,000 lines of code. I don't know if you spend much time in DMD's codebase, but I do, and it's drudgery navigating files of that size. Furthermore, since modules are D's unit of encapsulation, there's little encapsulation and separation of concerns in the semantic logic. That not only makes it difficult to see relationships between functions in all the noise, but it invites abuse as some functions cannot be privately restricted to just their intended callers.

It actually adds code!

That's not necessarily a bad thing, especially if some of that additional code is documentation that didn't previously exist.

@thewilsonator, I don't know if we'll be able to move forward with this given @andralex's comment. Though I would consider moving this forward if others voiced their support. I haven't actually looked much at the code though; not much point given @andralex's comment.

For now, I suggest focusing on bug fixes without the refactoring. Also, I recommend not reusing PRs when there is a significant change in its purpose; it tends to poison the conversation and review.

[thewilsonator]

@JinShil I take Andrei's comment along the lines of his "Don't let bureaucracy stand in the way of progress" comment, i.e. misinformed at best. The whole point of this refactoring was to allow the bug fix to happen, either I put the bug fix with the PR and Walter tells me to break it apart, or Andrei tells me that the refactoring is useless.

@ibuclaw sorry, you can fix your pragma issues on your own.

[andralex]

Sorry if I'm missing context or being hasty folks. What I saw in this PR was a reshuffling of the code that effects no visible improvement. The runes were in some position, they get thrown in the air, they land in a different position. As a litmus test, if the refactoring went the other way, would it be seen as an obvious problem?

The description could help. "Move pragma semantic analysis into its own module for the purpose of xxx" etc etc. The module addition is worthwhile if properly motivated. How does it enable fixing @ibuclaw 's pragma issues?

[thewilsonator]

Well the comments in the original that say //Should be merged with PragmaStatement and //Should be merged with PragmaDeclaration for a start. Having them separate makes is much easier to get out of sync e.g. w.r.t handling "unrecognised" pragmas.

See issue 19109,19110 and others in this list

[WalterBright]

Note that this function returns a void, and the overload of it returns a Statement. This certainly looks peculiar, and they shouldn't be overloads if they are doing fundamentally different things.

[thewilsonator]

Thats because the statement is needed in the Statement semantic, but the declaration is not needed for Declaration semantic. Why, I don't know.

[WalterBright]

Because of the distinctly different interfaces, they should have different names, not be overloads.

Overloads are useful for writing generic code, this isn't generic.

[WalterBright]

Not sure why a global .error call, given that above there's a custom error.

[thewilsonator]

Good point, I think I was confused about the loc parameter. It should fit the custom one.

[WalterBright]

The purpose of this struct appears to be to simply group together some related functions. Isn't that what modules are for, and the whole purpose of creating this file?

[ibuclaw]

How does it enable fixing @ibuclaw 's pragma issues?

Decentralise handling of D pragmas by enabling compiler vendors to add their own pragmas without modifying the dmd code.

The crux of it comes down to asking "Do you support this pragma?" before issuing an error.

I don't think you need to do a large refactoring in order to make this happen, but code to support it will be identical for both pragma declarations and statements.

@thewilsonator same as in your other patch for function semantic. Probably best to move these one at a time, and preferably to free functions so you aren't relying on some internal state.

[ibuclaw]

Sorry if I'm missing context or being hasty folks. What I saw in this PR was a reshuffling of the code that effects no visible improvement. The runes were in some position, they get thrown in the air, they land in a different position. As a litmus test, if the refactoring went the other way, would it be seen as an obvious problem?

@andralex - Under "Modernize DMD"

https://github.com/dlang/dmd/projects

In particular, this point applies:

Reduce cyclomatic complexity

I also take that to mean: Moving functionality into leaf modules in order to reduce cyclic dependencies of the compiler implementation.

And if it doesn't, then maybe we should add a new bullet to make that clearer.

An example, modifying dmd.argtypes will only trigger a rebuild of dmd.target, and modifying dmd.target will leave dmd.argtypes alone. I consider this as good encapsulation of a module, however, eyes of the beholder may apply.

[andralex]

@ibuclaw totally cool with the idea, and we should avoid entering a pattern whereby the slightest challenge is immediately met with abandoning the pull request.

"Reducing cyclomatic complexity" is a bit vacuous and should probably be changed because we currently have no way of measuring it. (It's more related to control flow paths than module dependencies though.)

Anyhow, unless a refactoring has obvious overwhelming advantages it should not just go through with just the "Refactoring Xxx" title and on the basis it passes tests. The code has a number of issues that do warrant a close review.

For example, the best thing about this PR is unifying two large horrendous functions. One is void visit(PragmaDeclaration pd), which opens with the comment // Should be merged with PragmaStatement, and of course the other corresponding void visit(PragmaStatement ps), which too, opens with the comment // Should be merged with PragmaDeclaration. The functions have 230 lines and 142 lines, respectively. Merging them properly together would be an obvious win. (I'm not sure if the right way is to have essentially two references PragmaStatement ps and PragmaDeclaration pd of which only one can be non-null, and then choose between them at runtime.) At any rate, a good refactor would do this kind of merge and be able to show a clear improvement, and would be the basis of any further work. @thewilsonator try that one?