Page MenuHomePhabricator

Added PublicOnly flag

Authored by anniecherk on Jun 20 2018, 2:32 PM.



Added a flag which, when enabled, documents only those methods and fields which have a Public attribute.

Diff Detail

Event Timeline

anniecherk created this revision.Jun 20 2018, 2:32 PM

Can you re-upload the patch with context? (i.e. use -U999999 or similar)

So on seeing this, all of the plumbing needed to get that particular value where you need it is a bit ugly. I'm inclined to suggest wrapping it in a ClangDocContext, containing it and the ExecutionContext in the ClangDoc.h file:

struct ClangDocContext {
    ExecutionContext &ECtx;
    bool PublicOnly;

and then basically just replace everywhere the ECtx is plumbed through with the new context object.

307 ↗(On Diff #152169)

You also want to check access here (records can be private to a class).

334 ↗(On Diff #152169)

Move this to the top of the function and do the check before you set the value on I -- if we're not going to return anything, you should exit quickly before you do all of the allocation.

340 ↗(On Diff #152169)

Also check here

68 ↗(On Diff #152169)

Can we make the flag just --public?

10 ↗(On Diff #152169)

Add a test case for a function.

  • addressed Julie's comments
  • updated logic to consider both access specifier & linkage
  • added tests for each type of linkage
anniecherk updated this revision to Diff 153790.Jul 2 2018, 2:10 PM

Updated the tests to more accurately check that the files that we expect to not be generated by clang-doc with the public flag are in fact not being created.

The tests now run clang-doc over the test files with and without the public flag, outputting yaml files into two different directories.
File-check then diffs those directories and verifies that the files we expect to be absent in the directory created from running clang-doc with the public flag are in fact absent.

juliehockett added inline comments.
27 ↗(On Diff #153790)

Can we put this in Representation.h? Here, you have a potential circular include with Mapper.h.

37–40 ↗(On Diff #153790)

Since the emitInfos are returning "" if they're being passed over, the results container is still storing them -- move the emission outside of this call, and only report if it returns a non-empty string.

23 ↗(On Diff #153790)

See above, I don't love the potential for circular includes (since Mapper.h is included in the implementation file for ClangDoc

174 ↗(On Diff #153790)

For readability, it's usually better to use positives (i.e. isPublic). Then, if we ever want to check for internal only, isPublic() is much more readable than !shouldNotBePublic().

311–313 ↗(On Diff #153790)

You can inline this.

69 ↗(On Diff #153790)


14 ↗(On Diff #153790)

I'm working on making these less brittle (so changing the USR schema doesn't require changing all these tests and just check the length with a regex, so let's just do that here (see D48341's tests for an example).

18 ↗(On Diff #153790)

To be system-agnostic, make this {{.*}} to match regardless of from where the test is run.

11–15 ↗(On Diff #153790)

Unnecessary whitespace

anniecherk updated this revision to Diff 153825.Jul 2 2018, 4:25 PM
anniecherk marked 4 inline comments as done.

Addressed Julie's comments

Please run clang-format and clang-tidy on the patch.

25 ↗(On Diff #153825)

Sort includes (this should be at the top)

13 ↗(On Diff #153825)

Sort includes alphabetically, but you shouldn't need this here?

23 ↗(On Diff #153825)

Sort includes (this should be at the top)

174 ↗(On Diff #153825)

clang-tidy will tell you to capitalize these in llvm world (as -> AS, link -> Link)

322–324 ↗(On Diff #153825)

Since this is the same for Record/Function/Enum, can we move this to populateSymbolInfo()?

Also, elide braces on single-line ifs.

anniecherk marked 11 inline comments as done.Jul 3 2018, 11:16 AM
anniecherk added inline comments.
322–324 ↗(On Diff #153825)

I don't see a good way to put it into populateSymbolInfo because if the condition passes then the emitInfo method needs to bail out and I don't see a clean way to do that if the check is in populateSymbolInfo. A clunky way to do this would be to either have populateSymbolInfo set a flag that emitInfo checks or emitInfo can assume that populateSymbolInfo bailed if all the info is unpopulated, but that seems like a worse way to do it. Am I missing something?

I can refactor the condition into a function if that would be better for understandability / maintainability.

juliehockett added inline comments.Jul 3 2018, 2:43 PM
322–324 ↗(On Diff #153825)

Hmm I suppose this is fine then.

Still elide braces on single-line ifs.

anniecherk updated this revision to Diff 154470.Jul 6 2018, 4:10 PM

Updated according to Julie's suggestions, mostly adjusted formatting.

juliehockett accepted this revision.Jul 11 2018, 8:55 AM

Remember to mark comments as done when they are. Otherwise, LGTM unless @ioeric has any concerns.

This revision is now accepted and ready to land.Jul 11 2018, 8:55 AM
anniecherk marked 5 inline comments as done.Jul 11 2018, 9:25 AM

Remember to mark comments as done when they are. Otherwise, LGTM unless @ioeric has any concerns.

No concern if this looks good to Julie.

14 ↗(On Diff #154470)

This could be [0-9A-Z]{N} where N = length(USR), right?

juliehockett added inline comments.Jul 16 2018, 2:55 PM
14 ↗(On Diff #154470)

The {n} syntax doesn't work with FileCheck.

This revision was automatically updated to reflect the committed changes.