On Tue, Jun 9, 2026 at 9:33 AM Markus Armbruster <[email protected]> wrote: > > John Snow <[email protected]> writes: > > > On Wed, Jun 3, 2026 at 7:27 AM Markus Armbruster <[email protected]> wrote: > >> > >> Quick first pass... > >> > >> John Snow <[email protected]> writes: > >> > >> > A forthcoming patch removes the implicit PLAIN section that always > >> > starts a QAPIDoc section list. Further future changes begin converting > >> > "PLAIN" sections to "INTRO" sections. To accommodate this, the > >> > insertion algorithm that places stub and dummy members must be > >> > adjusted to cope with not only the finished state, but temporary > >> > intermediate states while the series is merged. > >> > >> Perhaps: > >> > >> A forthcoming patch removes the implicit PLAIN section that always > >> starts a QAPIDoc section list. Further future changes begin converting > >> "PLAIN" sections to "INTRO" sections. > >> > >> This will affect the code that inserts "Not documented" descriptions > >> for undocumented members ("stub sections") and the dummy section that > >> marks the spot for "The members of ..." references. > >> > >> Adjust the algorithm to cope with not only the finished state, but > >> temporary intermediate states while the series is merged. > > > > Sure. > > > >> > >> > This algorithm can handle zero-or-more PLAIN *or* INTRO sections at > >> > the beginning of a QAPIDoc object, in contrast to the previous > >> > algorithm which assumed and relied upon there being always one PLAIN > >> > section at the beginning of every QAPIDoc section list. > >> > > >> > In other words: (PLAIN | INTRO)* <EverythingElse> > >> > > >> > This does not impact what the parser itself will actually produce. As > >> > of this patch, the parser will still always generate QAPIDoc section > >> > lists that start with precisely one PLAIN section (whether or not it > >> > is empty), followed by the remaining sections. Those remaining > >> > sections may or may not include additional PLAIN sections, but never > >> > two such sections contiguously as the parser will always treat that > >> > layout as one PLAIN section consisting of multiple paragraph(s). > >> > > >> > In other other words: This insertion algorithm is more lenient than > >> > the parser, but this is on purpose for flexibility mid-stream as we > >> > convert QAPI to using explicit introductory sections. The allowed > >> > order of sections will eventually become strictly enforced in the > >> > parser, which will in turn allow dramatic simplifications to the > >> > insertion algorithm. This only exists as transitory code until we are > >> > able to enforce that order. > >> > > >> > Fear not: the intermediate rest output before and after this patch > >> > >> "ReST output"? > > > > Sure > > > >> > >> > are byte identical, so failing all else, we at least know it doesn't > >> > make anything worse. > >> > > >> > Lastly, because we have three places in the code that need to insert > >> > stub/dummy sections, we take the opportunity to consolidate this code > >> > to handle all three cases with one function. This winds up > >> > necessitating the qapidoc.py generator actually modify the section > >> > list to insert a "dummy" member that acts as a placeholder for "The > >> > members of ..." text. While it looks like a code smell to modify the > >> > caller's argument, it is ultimately safe because the QAPI Schema > >> > object is re-parsed and re-constructed in memory for each individual > >> > process that needs to operate on it. In other words, the Sphinx > >> > document generator already does have "its own copy" of the section > >> > lists, so it is "safe" to modify here without regards to other > >> > consumers of the QAPIDoc objects. It only *looks* like it smells > >> > bad. Ultimately, this code will also be removed once the inliner is > >> > merged, so it is only a temporary aesthetic issue regardless. > >> > >> Such trickery, even when safe, risks making attentive readers go > >> "WAT?!?" I find it tolerable only because we plan to replace it fairly > >> soon. > >> > >> The code finding the spot to insert stub/dummy sections is more > >> complicated than it has any right to be, both before and after your > >> patch. It reconstructs information the doc parser has, but doesn't pass > >> on in usable form. Passing that info on would likely be simpler and > >> cleaner. However, you tell me your inliner patches also simplify > >> things, and they already exist. So let's go with those. > > > > Right, the basic idea is this: > > > > - Differentiate INTRO with new syntax > > - Convert "PLAIN" into "DETAILS" > > - Enforce strict section ordering: INTRO only at the beginning, > > DETAILS only at or quite near to the end > > > > Once that is cemented, the insertion algorithm can become much simpler > > and dumber; e.g. using a map of regions where we can append to the end > > of a region without having to iterate through to find our sweet spot. > > > > Since that's the plan for the inliner anyway, some temporary ugliness > > here in order to ensure that we do not regress even one byte seems > > acceptable. > > > > Additionally, once the inliner is merged, we won't need the "dummy" > > sections at all, so those go away entirely, and all of the ugliness of > > that particular part of the hack with it - i.e. no more modifying the > > caller's argument to insert a bookmark. > > > > So, it's a little gross, but it's just a band-aid to get to where we > > want to be; and it isn't as gross as it looks. > > > > With your suggested commentary changes, do I have your blessing to > > push forward? In the interim, please review the rest of the series so > > I can send you a new version. > > Yes, you do. > > In addition to the commit message changes, I'd like to have a comment in > the code. > > Perhaps as separate patch before your series:
It's easy enough to add it as a separate patch right before this one instead of in a separate series. Added in with no rebasing issues, so I'm going to do it that way just to avoid any potential merging issues. > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index c3cf33904e..844afc6e0e 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -852,6 +852,12 @@ def connect_feature(self, feature: 'QAPISchemaFeature') > -> None: > > def ensure_returns(self, info: QAPISourceInfo) -> None: > > + # This is more complicated than it ought to be. The doc > + # parser should already know where a generated RETURNS section > + # should go. It currently doesn't, mostly because it accepts > + # tagged sections in any order. > + # TODO Tighten doc syntax and simplify. > + > def _insert_near_kind( > kind: QAPIDoc.Kind, > new_sect: QAPIDoc.Section, > -- > 2.54.0 > > We could adjust the comment within your series to also cover the new > uses of _insert_near_kind(), but I don't think that's necessary. It > should serve as a reminder as is. > > If that works for you, I'll post the patch. > > > > > --js > > > >> > >> > That's my story and I'm sticking to it. > >> > > >> > Signed-off-by: John Snow <[email protected]> > >> >
