encapsulate CSE #9993
encapsulate CSE #9993
Conversation
|
Thanks for your pull request, @WalterBright! Bugzilla referencesYour 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 locallyIf 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#9993" |
|
With this PR all methods are static and all data is static in |
| } | ||
|
|
||
|
|
||
| /******************** | ||
| * The above implementation of CSE is inefficient: | ||
| * 1. the optimization to not store CSE's that are never reloaded is based on the slot number, |
There was a problem hiding this comment.
None of the functions have a Ddoc Params section.
| } | ||
|
|
||
| private: | ||
| __gshared: |
There was a problem hiding this comment.
I would remove the indentation of these. We never have an indentation of two spaces, as far as I know.
There was a problem hiding this comment.
I've used it many times for attributes.
| /****************** | ||
| * Returns: | ||
| * total size used by CSE's | ||
| */ |
There was a problem hiding this comment.
I think you can use the more compact Ddoc in the form of /// as has been done for offset.
|
I do see now that you have non-static data. Would it be better to separate the static from the non-static data? |
|
|
||
| /*** | ||
| * Finish generating code for this function. | ||
| * Get rid of unused cse temporaries by shrinking the array. |
There was a problem hiding this comment.
Please add a newline before this line. The first line will be a summary and should be short.
|
|
||
| bool empty() | ||
| { | ||
| while (i) |
There was a problem hiding this comment.
I think a foreach_reverse would be simpler. When reading code and you see the while keyword you have no idea of what it's doing. While when reading the foreach_reverse keyword you know that in 99% percent of the cases it's going to iterate a data structure in reverse. The difference here is that with while you need to read the whole body but with foreach_reverse you only need to read the keyword.
There was a problem hiding this comment.
On the other hand, many people complain about foreach_reverse. The loop is trivial enough I don't think it matters, and can't see how people would be confused about it.
There was a problem hiding this comment.
On the other hand, many people complain about foreach_reverse
Yes, and I'm one of them. I have complained that it's a dedicated keyword. It could have been implemented like foreach.reverse, in which case only foreach would be a keyword. Or not have any language support at all and instead rely on a library solution like std.range.retro.
| struct Range | ||
| { | ||
| const elem* e; | ||
| int i; |
There was a problem hiding this comment.
Why is int used and not size_t?
There was a problem hiding this comment.
At one point I had it going to -1.
I thought about it. I didn't think it would improve things.
That was my original plan, but it didn't seem to improve anything. |
In it's current form, no, I agree. But for the future, assuming we want to remove global data (which the static data inside |
|
the buildkite failure seems spurious |
This is not perfect, but it's what I mean by encapsulation:
If I did this right, the deficiencies of the CSE storage methods (outlined at the end of cgcse.d) can be addressed without changing any file other than cgcse.d.