search

kuznia-rdzeni / coreblocks

RISC-V out-of-order core for education and research purposes
https://kuznia-rdzeni.github.io/coreblocks/
BSD 3-Clause "New" or "Revised" License
36 stars 14 forks source link

Docs for add_conflict #252

Open Kristopher38 opened 1 year ago

Kristopher38 commented 1 year ago
  1. Info from TransactionBase isn't present in the docs so add_conflict isn't visible there. Possibly more such cases
  2. Meaning of priority argument to add_conflict isn't documented. What does it mean that the priority is left/right/undefined?
tilk commented 1 year ago
  1. There is a Sphinx directive inherited-members which shows inherited members for a class. When applied indiscriminately through sphinx-apidoc, we get the opposite problem: docs show methods and attributes which should not be there. I did not find a simple way to apply inherited-members just to Transaction and Method. If someone knows Sphinx better than me, go ahead.
  2. I thought it was self-documenting: LEFT means that left transaction/method is prioritized over the right, RIGHT is the reverse, UNDEFINED means that there is no prioritization, just a conflict. I can make a small PR with appropriate comments if you want.
Kristopher38 commented 1 year ago

I thought it was self-documenting: LEFT means that left transaction/method is prioritized over the right, RIGHT is the reverse

Am I missing some fundamental knowledge? Where did that come from?

Kristopher38 commented 1 year ago

Oh... I think I get it. "left" and "right" as in a.add_conflict(b) - Priority.LEFT would mean a > b, and Priority.RIGHT the reverse? If so, that wasn't obvious at all! Maybe because I haven't seen this kind of pattern before (?)

tilk commented 1 year ago

Indeed, that's what it means. Nobody opposed this API when it was merged ;) The alternative API would be to add a keyword Boolean parameter to schedule_before, which says if there is a conflict or not. I don't know if it is any better, and the problem is pure bikeshedding anyway ;)