Skip to content

Conversation

evelez7
Copy link
Member

@evelez7 evelez7 commented Aug 30, 2025

GSoC 2025 Improving Core Clang-Doc Functionality blog post

@evelez7
Copy link
Member Author

evelez7 commented Aug 30, 2025

cc: @petrhosek @ilovepi

Copy link

@ilovepi ilovepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few meta comments. First, you're underselling how much work you did in many ways. You completely rewrote a large portion of clang-doc, and solidified a new architecture that will allow us to delete a lot of code, with no performance loss and much better testing/maintainability.

second, because of your refactor you were able to implement several features really quickly. I don't think you hardly pointed those out at all.

Next, you wrote a lot of tests.

Lastly, we're almost ready to turn on the mustache backend on by default once we iron out 1-2 small issues w/ names. That's all because of you. Don't be modest, this is the time to brag about all the good things you did, because if you don't tell people, no one will know.

@ilovepi
Copy link

ilovepi commented Aug 30, 2025

I'd also consider adding a diagram or 2 if you have time/capacity.

@ilovepi
Copy link

ilovepi commented Aug 30, 2025

Oh, and mention that you're presenting at LLVM dev meeting! can't leave that out!

@evelez7 evelez7 requested a review from ilovepi September 1, 2025 08:56
Comment on lines +41 to +43
<div style="margin:0 auto;">
<img src="/img/gsoc-2025-clang-doc-architecture.png"><br/>
</div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what time constraints are for the post, but I'd make a contrasting diagram for the old architecture that shows there were several backends, and we're moving to an architecture where you basically have one backend and then the templating just adjusts the format. from the clang-doc perspective you'll always just go to JSON and then generate the output format based on the template. What those templates are are is kind of irrelevant, since you'll just pick a template and some path names based on the output type (eventually). I didn't really pick that up in the text here or below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no constraints. Take as much time as needed to polish the post :)

1. Visit source declarations via Clang's ASTVisitor.
2. Serialize relevant source information into an Info (Clang-Doc's main data entity).
3. Once all source declarations are serialized, write them into bitcode, reduce, and read the reduced Infos.
4. Serialize Infos into the desired format.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe provide more details here? the reader should have some intuition that these backends had a cumbersome job, and repeated lots of code to handle the same structures (and sometimes didn't!).

Comment on lines 45 to 50
It seems fairly straightforward, but the architecture had a critical flaw.
If a new C++ construct needed to be supported, it would be visited and serialized, but then support would have to be added to each backend individually.
If you wanted to serialize something in YAML, you'd have to implement the Markdown logic separately.
This imposed a very high maintenance cost for extending basic functionality, even if you just wanted to add something simple.
It also easily led to generator disparity; a construct might be serialized in YAML, but not in Markdown.
Testing was also in an awkward spot because it was unclear what format would be used to verify if the documentation output was acceptable.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're sort of talking around the issue: yes you'd need to implement a thing in multiple places, but you're not highlighting that all the basic logic for handling any kind of Info was basically reimplemented for each backend, with almost no sharing (and some of them weren't implemented the same way). That's significantly worse than just handling things per backend. LLVM contributors won't have a feel for why this is bad unless you spell it out in a way that they'll see this isn't just lkie having an x86 backend and an Aarch64 backend w/ a bit of overlap. Its that, plus they each have their own implementations of all the shared logic in CodeGen. In fact, you may want to spell it out that way, since its an analogy that contributors will readily understand.

It also easily led to generator disparity; a construct might be serialized in YAML, but not in Markdown.
Testing was also in an awkward spot because it was unclear what format would be used to verify if the documentation output was acceptable.

## The Good: Mustache
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expected to see sections with The Good:, The Bad:, The Ugly: (or at least the first two). But

@ilovepi
Copy link

ilovepi commented Sep 12, 2025

@evelez7 any updates? Several other posts have gone up so far.

@evelez7
Copy link
Member Author

evelez7 commented Sep 12, 2025

@evelez7 any updates? Several other posts have gone up so far.

Yeah I have a large change for organization and content that addresses the last round of feedback. Gives more context and better flow I think. Pushing it tonight, sorry about the delay.

@evelez7 evelez7 requested a review from ilovepi September 12, 2025 20:07
@evelez7
Copy link
Member Author

evelez7 commented Sep 12, 2025

I added a new diagram to show the problems with the old architecture, expanded detail in the bitcode refactor section, expanded details in the "bad" architecture section with a code example of the repeated yet dissimilar logic, and rearranged some of the sections a bit.

Copy link

@ilovepi ilovepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a lot better. I'd still love to see some more details, but lets save something for the dev meeting :)

@petrhosek do you have any further thoughts?

@evelez7 evelez7 requested a review from petrhosek September 15, 2025 18:38
Copy link

@ilovepi ilovepi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is probably in a decent state now. I'd encourage you give it another read through before landing to make sure you don't want to change anything.

Copy link
Contributor

@asl asl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a decent post to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants