Page MenuHomePhabricator

[clang-tidy] add options documentation to readability-identifier-naming checker
ClosedPublic

Authored by MyDeveloperDay on Jan 10 2019, 2:54 PM.

Details

Summary

The documentation for this clang-checker did not explain what the options are. But this checkers only works with at least some options defined.

To discover the options, you have to read the source code. This shouldn't be necessary for users who just have access to the clang-tidy binary.

This revision, explains the options and gives an example.

Diff Detail

Repository
rL LLVM

Event Timeline

MyDeveloperDay created this revision.Jan 10 2019, 2:54 PM

Thank you for keeping documentation up-to-date with code!

I think will be good idea to create generic Case/Prefix/Suffix description to reduce size of documentation.

docs/clang-tidy/checks/readability-identifier-naming.rst
34 ↗(On Diff #181163)

check, please. Same in other places.

61 ↗(On Diff #181163)

I think will be good idea to indent public: same way as LLVM style does. Same in other places.

278 ↗(On Diff #181163)

Will be good idea to run Clang-format over code snippets.

Eugene.Zelenko set the repository for this revision to rCTE Clang Tools Extra.Jan 10 2019, 3:17 PM
Eugene.Zelenko added a subscriber: cfe-commits.

Address review comments

  • clang-format the code examples
  • replace "ensure" with "check"
MyDeveloperDay marked 5 inline comments as done.Jan 11 2019, 2:02 AM

I think will be good idea to create generic Case/Prefix/Suffix description to reduce size of documentation.

I'm generating this documentation via a script, using the following template, (which is why its quite repetitive), the grunt work was in making the examples which are stored in separate Option_Before.cpp/Option_After.cpp file.

This was actually my second pass, initially I called out each option individually but that made for 100's of example files, if you think this text could be generalized even more I'd be happy to regenerate it. (I'm not much of a wordsmith)

.. option:: %name%Case

When defined, the checker will check %desc% names conform to the
selected casing.

.. option:: %name%Prefix

When defined, the checker will check %desc% names will add the
prefixed with the given value (regardless of casing).

.. option:: %name%Suffix

When defined, the checker will check %desc% names will add the
suffix with the given value (regardless of casing).

For example using values of:

  • %name%Case of `lower_case`
  • %name%Prefix of `pre_`
  • %name%Suffix of `_post`

    Identifies and/or transforms %desc% names as follows:

    Before:

    .. code-block:: c++ %before_example%

    After:

    .. code-block:: c++ %after_example%
docs/clang-tidy/checks/readability-identifier-naming.rst
34 ↗(On Diff #181163)

I assumed you mean replace "ensure" with "check", now I'm not sure do you mean?

A) "the checker will check"
B) "the check will ensure"
C) "the check will check"

278 ↗(On Diff #181163)

so I formatted the examples but public didn't get indented, let me know if you think I've got the format wrong

MyDeveloperDay marked 2 inline comments as done.Jan 11 2019, 4:42 AM
docs/clang-tidy/checks/readability-identifier-naming.rst
34 ↗(On Diff #181163)

I'm sorry for been ambiguous. I meant replacing checker with check. Actually this wider problem: check is in Clang-tidy; checker - in Static Code Analyzer.

Fix review comments

s/the checker will check/the check will ensure/

MyDeveloperDay marked an inline comment as done.Jan 14 2019, 12:03 AM

Thank you very much for working on this!
See https://bugs.llvm.org/show_bug.cgi?id=34990 for the bug report, I mentionend this revision.

TBH i did not read through all the document, but scrolled mostly starting from 25%, it looks good to me.
Given its length, what do you think about a short link-list at the beginning that will point to the section in the docs? With that its easier to see whats all handled by the check.

docs/clang-tidy/checks/readability-identifier-naming.rst
9 ↗(On Diff #181497)

It supports one of the following, i think the of is missing?

26 ↗(On Diff #181497)

kinds of identifiers? second 's'

70 ↗(On Diff #181497)

That would have been a change of the constructor name, the other examples don't have this behaviour, probably copy&paste artifact?

MyDeveloperDay marked 3 inline comments as done.

Addressing review comment

Given its length, what do you think about a short link-list at the beginning that will point to the section in the docs? With that its easier to see whats all handled by the check.

How about the following...

That looks good.

This revision is now accepted and ready to land.Jan 15 2019, 12:33 PM
This revision was automatically updated to reflect the committed changes.