bitfield diagram changes #5
Conversation
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
Here are a few suggestions! The main thing here is that the JavaScript code could really use some more comments—some of the uncommented functions are hard to follow without more context.
| } | ||
| } | ||
|
|
||
| const bitfieldJSON = { |
There was a problem hiding this comment.
I think this part could use some comments. What exactly is bitfieldJSON? What fields are present, and what is it eventually used for?
.eleventy.js
Outdated
| return String(range); | ||
| }); | ||
|
|
||
| eleventyConfig.addPassthroughCopy({ "node_modules/bit-field": "bitfield" }); |
There was a problem hiding this comment.
Could we just include the .../build subdirectory instead of the whole directory? Seems like that's all we're actually using (in the <script> tag above).
src/instructions/instruction.njk
Outdated
| } | ||
| function renderBitfieldScripts() { | ||
| const scripts = document.querySelectorAll('script[type="text/bitfield"]'); |
There was a problem hiding this comment.
I'm a tiny bit confused about the overall strategy here. Maybe it just needs more comments? I think what you're doing is finding a way to embed the underlying JSON data in a way that the frontend will then parse? And the <script> tag is a hack for embedding that JSON string?
A little bit of commenting here could go a long way!
src/instructions/instruction.njk
Outdated
| try { | ||
| const payload = JSON.parse(raw); | ||
| if (Array.isArray(payload)) { | ||
| return { segments: payload }; | ||
| } | ||
| if (payload && Array.isArray(payload.reg)) { | ||
| return { segments: payload.reg, options: payload.options }; | ||
| } | ||
| if (payload && Array.isArray(payload.fields)) { | ||
| return { segments: payload.fields, options: payload.options }; | ||
| } |
There was a problem hiding this comment.
Can you please explain a bit more about why we need these three cases? I guess I don't understand why we make the underlying raw JSON document have optional fields, like reg and fields, instead of just having it provide segments and options directly… it would be simpler if we could just assume that the JSON we parse always has all the data we need.
src/instructions/instruction.njk
Outdated
| const segments = parsed.segments | ||
| .map(seg => { | ||
| const bits = Number(seg.bits ?? seg.width ?? 0); | ||
| return Object.assign({}, seg, { bits }); | ||
| }) | ||
| .filter(seg => seg.bits > 0); | ||
| if (!segments.length) return; | ||
| const totalBits = segments.reduce((sum, seg) => sum + seg.bits, 0); | ||
| const options = Object.assign({ bits: totalBits }, parsed.options || {}); |
There was a problem hiding this comment.
Similarly to the above, I don't think I quite follow why this logic is necessary. I get that we eventually need to compute the options to pass to bitfield.render, but it seems a little weird that those options are being built up in multiple locations: here, in parseBitFieldData, and potentially in the thing that generates the JSON. Can we centralize that so that this option computation only appears in one place? That would make it a LOT easier to read/maintain.
src/instructions/instruction.njk
Outdated
|
|
||
| <script> | ||
| (function () { | ||
| function jsonmlToDom(node, inSvg) { |
There was a problem hiding this comment.
I think I don't understand what this function does or why it's necessary. Can you please add some comments to describe overall what the arguments are and what it is supposed to return?
Here's my best guess so far. bitfield.render produces an SVG-like data structure, and we need to convert this into real SVG elements. It seems like this uses a particular standard-ish format for representing XML in JavaScript called JsonML. So you have hand-written a translator from that JsonML format to DOM elements.
But looking at the JsonML page, it looks like there are plenty of existing tools for converting from it to DOM nodes. And the bit-field README suggests using onml.stringify to get the SVG code, which seems really simple. Can we just do that, and use something like innerHTML to insert this SVG code into the page?
No description provided.