This is an archive of the discontinued LLVM Phabricator instance.

[ASTMatchers] Add an isMain() matcher
ClosedPublic

Authored by george.karpenkov on Jul 20 2018, 1:43 PM.

Diff Detail

Repository
rC Clang

Event Timeline

aaron.ballman accepted this revision.Jul 23 2018, 5:00 AM

LGTM, though if you can split the unrelated changes out into a separate commit, that would be great.

clang/docs/LibASTMatchersReference.html
3284 ↗(On Diff #156599)

It seems like there are a lot of unrelated changes in the patch. If you could submit these separately, that'd be great -- it doesn't require review.

This revision is now accepted and ready to land.Jul 23 2018, 5:00 AM
clang/docs/LibASTMatchersReference.html
3284 ↗(On Diff #156599)

that's because there's no mechanism enforcing HTML generation before the commit.
Why do we even have HTML in the repo? Couldn't that just be generated by the website on post-receive hook?

aaron.ballman added inline comments.
clang/docs/LibASTMatchersReference.html
3284 ↗(On Diff #156599)

Auto-generated documentation is a newer concept; I suspect we could do it here as well and stop committing the HTML, but no one's worked on such functionality. Personally, I'd be happy to see that functionality added if it doesn't cause problems on the server side of things. If you're interested in working on something like that, I'd recommend starting a thread on cfe-dev and be sure to include @tonic as she did the infrastructure for attribute and command line documentation generation.

@aaron.ballman Right, I see. I don't have resources to work on that though.

My bigger point was that under the current setup it's extremely easy to forget to update the HTML comments,
and many (if not most) of the commits do that.
Then it's completely unreasonable to put the burden of separating HTML changes into separate reviews
on a person who actually does do the right thing and runs the documentation generator.
If we don't have facilities for proper automatic documentation generation then we should just treat diffs on those as ignorable noise.

@aaron.ballman Right, I see. I don't have resources to work on that though.

My bigger point was that under the current setup it's extremely easy to forget to update the HTML comments,
and many (if not most) of the commits do that.
Then it's completely unreasonable to put the burden of separating HTML changes into separate reviews
on a person who actually does do the right thing and runs the documentation generator.
If we don't have facilities for proper automatic documentation generation then we should just treat diffs on those as ignorable noise.

I understand that perspective and somewhat agree with it. However, we don't typically allow "drive-by" fixes in other areas to come in as part of a commit, either. It makes code archaeology harder when there are unrelated parts of a commit that you have to research long after the commit happens.

This issue should be solved by making it an impossible situation, but until someone puts in that effort, the burden should remain on patch authors to generate the documentation and reviewers should continue to insist on the documentation being generated prior to committing any changes. This particular set of problems seems to have stemmed from r337209 because the HTML generated and committed with that commit was stale compared to what was in ASTMatchers.h. It seems we both messed that one up (it was your commit and my LG).

@aaron.ballman

It seems we both messed that one up

Not only though: there's also the typo fixed in hasUnqualifiedDesugaredType.

OK I'll separate this one.

@aaron.ballman

It seems we both messed that one up

Not only though: there's also the typo fixed in hasUnqualifiedDesugaredType.

Hah, good catch!

OK I'll separate this one.

Thank you.

This revision was automatically updated to reflect the committed changes.