This is an archive of the discontinued LLVM Phabricator instance.

[PATCH] Bug 27475 - Request header guard check processes .hpp files as well as .h files
ClosedPublic

Authored by madsravn on May 21 2016, 1:38 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

madsravn updated this revision to Diff 58044.May 21 2016, 1:38 PM
madsravn retitled this revision from to [PATCH] Bug 27475 - Request header guard check processes .hpp files as well as .h files .
madsravn updated this object.
madsravn added reviewers: alexfh, flx.
madsravn added a subscriber: cfe-commits.
hokein added a subscriber: hokein.May 23 2016, 7:31 AM
hokein added inline comments.
clang-tidy/llvm/HeaderGuardCheck.h
19 ↗(On Diff #58044)

You should add a document for the option HeaderFileExtensions here.

clang-tidy/utils/HeaderGuard.cpp
300 ↗(On Diff #58044)

To avoid the redundant code, you can put this kind of code to utls::HeaderFileExtensionsUtils (something like isHeaderFileExtension).

clang-tidy/utils/HeaderGuard.h
15 ↗(On Diff #58044)

seems not needed?

45 ↗(On Diff #58044)

Not needed to be a member variable.

madsravn added inline comments.May 23 2016, 9:21 AM
clang-tidy/llvm/HeaderGuardCheck.h
19 ↗(On Diff #58044)

I'm not sure what you mean here. What is "a document" in this context?

hokein added inline comments.May 23 2016, 10:12 AM
clang-tidy/llvm/HeaderGuardCheck.h
19 ↗(On Diff #58044)

Sorry for the unclear comment. You actually add an option HeaderFileExtensions in LLVMHeaderGuardCheck, so you need to add a document to describe it. See the file DefinitionsInHeadersCheck.h for details.

alexfh added inline comments.May 24 2016, 4:30 PM
clang-tidy/llvm/HeaderGuardCheck.cpp
18 ↗(On Diff #58044)

This should be done in the constructor, not on each call to this method.

alexfh requested changes to this revision.May 27 2016, 6:58 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.May 27 2016, 6:58 AM
hokein added inline comments.Jun 7 2016, 5:17 AM
clang-tidy/llvm/HeaderGuardCheck.h
19 ↗(On Diff #58044)

You need to add a comment here, like:

/// Finds and fixes header guards that do not adhere to LLVM style.
///
///  The check supports these options:
///   - `HeaderFileExtensions`: a comma-separated list of filename extensions of
///     header files (The filename extension should not contain "." prefix).
///     ",h,hh,hpp,hxx" by default.
madsravn updated this revision to Diff 69131.Aug 24 2016, 9:38 AM
madsravn edited edge metadata.
madsravn marked 8 inline comments as done.

Updated the patch as suggested by hokein and alexfh.

alexfh requested changes to this revision.Aug 24 2016, 10:17 AM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/llvm/HeaderGuardCheck.cpp
18 ↗(On Diff #69131)

nit: In LLVM/Clang code it's not common to enclose single-line if/while/for bodies in braces.

18 ↗(On Diff #69131)

if (extension.startswith("."))

22 ↗(On Diff #69131)

return HeaderFileExtensions.count(extension) > 0;

26 ↗(On Diff #69131)

Were you going to use isHeaderFileExtension, btw?

clang-tidy/utils/HeaderFileExtensionsUtils.cpp
64 ↗(On Diff #69131)
  1. clang-format the code
  2. see comments for LLVMHeaderGuardCheck::shouldFixHeaderGuard above
This revision now requires changes to proceed.Aug 24 2016, 10:17 AM
madsravn updated this revision to Diff 69143.Aug 24 2016, 11:12 AM
madsravn edited edge metadata.

Fixed suggested by alexfh

Thank you! I've just noticed that we had completely ignored the actual docs =\ See the inline comment for details.

clang-tidy/llvm/HeaderGuardCheck.h
20 ↗(On Diff #69143)

Please add

/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html

and copy the rest of the comment to the corresponding .rst file with proper formatting (see docs/clang-tidy/checks/misc-argument-comment.rst for an example).

clang-tidy/utils/HeaderFileExtensionsUtils.h
44 ↗(On Diff #69143)

nit: Add a trailing period.

alexfh requested changes to this revision.Aug 24 2016, 11:55 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Aug 24 2016, 11:55 AM
madsravn updated this revision to Diff 69152.Aug 24 2016, 12:14 PM
madsravn edited edge metadata.
madsravn marked 5 inline comments as done.

More suggestions by alexfh fixed.

alexfh accepted this revision.Aug 24 2016, 12:33 PM
alexfh edited edge metadata.

LG with a couple of nits. Do you need me to submit the patch for you? (If yes, I can fix the doc myself)

docs/clang-tidy/checks/llvm-header-guard.rst
8 ↗(On Diff #69152)

This line is not needed here (it's a self-reference).

10 ↗(On Diff #69152)

Should be

Options
-------

.. option:: HeaderFileExtensions

  ...
This revision is now accepted and ready to land.Aug 24 2016, 12:33 PM
madsravn updated this revision to Diff 69158.Aug 24 2016, 12:55 PM
madsravn edited edge metadata.
madsravn marked 2 inline comments as done.

Documentation fix.

I have commit access. I can commit it myself :) thanks though.

Is the documentation correct? I wasn't sure about the "\n ..." part.

clang-tidy/utils/HeaderGuard.h
15 ↗(On Diff #58044)

Needed for Options.

alexfh added inline comments.Aug 24 2016, 1:13 PM
docs/clang-tidy/checks/llvm-header-guard.rst
13 ↗(On Diff #69158)

... was meant to represent the description of the option. Not literally ... ;)

The description should be indented by at least two columns and should start with A comma-separated ...

madsravn updated this revision to Diff 69164.Aug 24 2016, 1:24 PM
madsravn added inline comments.
docs/clang-tidy/checks/llvm-header-guard.rst
13 ↗(On Diff #69158)

Sorry :) I just found the literal ... in some of the other .rst files and it somewhat fit the context. I'll get it fixed.

alexfh added inline comments.Aug 24 2016, 1:49 PM
docs/clang-tidy/checks/llvm-header-guard.rst
13 ↗(On Diff #69164)

You missed the "The description should be indented by at least two columns" part.

With that should be fine to submit.

madsravn updated this revision to Diff 69250.Aug 25 2016, 8:12 AM

Last change - documentation should be fine now.

This revision was automatically updated to reflect the committed changes.