dmd icon indicating copy to clipboard operation
dmd copied to clipboard

mtype: Add TypeAggregate and refactor repetitive code for structs and classes

Open ljmf00 opened this issue 2 years ago • 10 comments

Signed-off-by: Luís Ferreira [email protected]


Previously reverted https://github.com/dlang/dmd/pull/13734 by https://github.com/dlang/dmd/pull/13742 . CC @ibuclaw .

ljmf00 avatar Mar 11 '22 01:03 ljmf00

Thanks for your pull request, @ljmf00!

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

dlang-bot avatar Mar 11 '22 01:03 dlang-bot

@ljmf00 @ibuclaw what's the status of this PR?

PetarKirov avatar Mar 16 '22 08:03 PetarKirov

@ljmf00 @ibuclaw what's the status of this PR?

The needless whitespace changes make it hard to vet that the functional changes to the C++ headers are OK.

ibuclaw avatar Mar 16 '22 22:03 ibuclaw

@ibuclaw Fixed. Can you re-review?

Also, I'm going to add a clang-format config proposal and try to fix the format inconsistency. It is utterly unproductive to copy and fix indentation by hand, especially the side of the pointer. There are also some types that don't correspond to the D type. It is not related to this change, so I'm going to do those in separate PRs.

ljmf00 avatar Mar 18 '22 18:03 ljmf00

Should be tested against either gdc or ldc (@kinke) to gauge how much of a nuance that asym field will be without an accessor.

ibuclaw avatar Mar 21 '22 19:03 ibuclaw

This needs rebasing on top of #13862.

ibuclaw avatar Mar 23 '22 12:03 ibuclaw

This needs rebasing on top of #13862.

Done. Do you want me to make sym an accessor? Probably an inline implementation on mtype.h is preferable.

ljmf00 avatar Mar 24 '22 19:03 ljmf00

This needs rebasing on top of #13862.

Done. Do you want me to make sym an accessor? Probably an inline implementation on mtype.h is preferable.

Could we instead turn this on its head and keep the sym fields in TypeClass and TypeStruct, instead having the accessor in TypeAggregate?

Also considering that getting the .sym from the former two nodes is in the hot path, and we want to avoid a systematic slowdown by a thousand cuts.

ibuclaw avatar Mar 24 '22 22:03 ibuclaw

ping @ljmf00

RazvanN7 avatar Apr 25 '22 13:04 RazvanN7

Could we instead turn this on its head and keep the sym fields in TypeClass and TypeStruct, instead having the accessor in TypeAggregate?

Also considering that getting the .sym from the former two nodes is in the hot path, and we want to avoid a systematic slowdown by a thousand cuts.

I think moving the asym to TypeClass and TypeStruct will make this change less useful. The accessors are there just to facilitate the casting, ultimately, they should be inlined. The choice of not having the accessor would require casting the asym to the respective type when using the specialization of either TypeClass or TypeStruct, but when dealing with TypeAggregate I would still have the abstraction.

If it's not what you mean, can you clarify?

ljmf00 avatar Apr 28 '22 20:04 ljmf00