search

snazzy-d / sdc

The Snazzy D Compiler
MIT License
246 stars 55 forks source link

Track dirty pages in regionAllocator. #338

Closed dsm9000 closed 9 months ago

dsm9000 commented 9 months ago

Cut-down split of https://github.com/snazzy-d/sdc/pull/335 : only the dirt tracking per se.

deadalnix commented 9 months ago

There is no test that the dirty pages are actually tracked properly. Making sure it is in all circumstance, in a data structure that make sense, is a big enough challenge, no reason to add more. However, it's a good reason to make sure this is well tested and correct in all circumstance, so it can serve as a layer fancier things can be built upon.

dsm9000 commented 9 months ago

@deadalnix The re-using logic and its test, were the bulk of the test for this. The remainder is e.g. here. What kind of test do you think it would be appropriate to add here while maintaining the given split?

deadalnix commented 9 months ago

With all the scenari that can happen and need to be kept track off, I'm confident that if this is mixed with reusing, the testing is not appropriate. Thankfully, if the PR is made complicated enough, this fact can be obfuscated.

dsm9000 commented 9 months ago

@deadalnix are you able to point to something concrete and say "this part is needlessly complicated, it is possible to fulfill the spec without it" or is your objection simply "too much green in this diff" ?

deadalnix commented 9 months ago

There is a ton of code that has nothing to do with what this PR tries to achieve. And there is code that clearly lost track of the dirty pages in there.

Which also means that the testing for all of this is inadequate, as tests shouldn't be passing when the code is doing the wrong thing.