This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] bugprone-header-guard : a simple version of llvm-header-guard
Needs ReviewPublic

Authored by trixirt on May 3 2019, 7:34 AM.

Details

Summary

A general use, missing header guard finder and fixer.
No style checks. If there is already a working header guard, it is used.
If the header guard is missing or broken, it is created from the path name
using this method.

<path-to>/foo.h ->

#ifndef FOO_H
#define FOO_H

#endif

The motivation for this change is fixing a large, non llvm with many missing header guards.
As no style is checked, it should not conflict with llvm-header-guard.

Fix docs formatting of all checks using with the HeaderFileExtensions option.

Diff Detail

Event Timeline

trixirt created this revision.May 3 2019, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2019, 7:34 AM
Eugene.Zelenko added inline comments.
clang-tidy/misc/HeaderGuardCheck.cpp
21 ↗(On Diff #198001)

Please use early return.

docs/clang-tidy/checks/misc-header-guard.rst
14 ↗(On Diff #198001)

Please use single back-tick instead of quotes for option values. Same below.

Eugene.Zelenko added a project: Restricted Project.
trixirt updated this revision to Diff 198146.May 4 2019, 9:59 AM
trixirt edited the summary of this revision. (Show Details)

Addressed issue with returning early and using backtick in doc.
Since misc-header-guard doc was a cut-n-paste of llvm-header-guard, that and other similar docs were changed.

Hi trixirt and thanks for the patch!

I would rather like to generalize the llvm check to allow different styles and then alias the general version with different configurations. Introducing this code duplication does not sound like a good idea to me.
The documentation fixes you make can be done in a separate patch to keep things clean.

Hi trixirt and thanks for the patch!

I would rather like to generalize the llvm check to allow different styles and then alias the general version with different configurations. Introducing this code duplication does not sound like a good idea to me.
The documentation fixes you make can be done in a separate patch to keep things clean.

I'd probably reverse that -- have bugprone (not misc) carry a general check for header guards with configuration options, and have the llvm check defer to the general check with specific configuration options.

clang-tidy/misc/HeaderGuardCheck.cpp
25–26 ↗(On Diff #198146)

This replacement may generate a header guard that exhibits UB. Consider a filename like foo-.cpp, which will become foo__cpp

trixirt updated this revision to Diff 198352.May 6 2019, 3:17 PM
trixirt retitled this revision from [clang-tidy] misc-header-guard : a simple version of llvm-header-guard to [clang-tidy] bugprone-header-guard : a simple version of llvm-header-guard.

Move from misc to bugprone

All of the real work is the header-guard checks is done by their common base class utils/HeaderGuard.
The much smaller llvm and bugprone subclasses are only concerned with the style of the fix.

My understanding of the directory layout guidance in the docs is that different styles get different directories.

Next, you need to decide which module the check belongs to. Modules
are located in subdirectories of `clang-tidy/
<https://github.com/llvm/llvm-project/tree/master/clang-tools-extra/clang-tidy/>`_
and contain checks targeting a certain aspect of code quality (performance,
readability, etc.), certain coding style or standard (Google, LLVM, CERT, etc.)
or a widely used API (e.g. MPI). Their names are same as user-facing check
groups names described :ref:`above <checks-groups-table>`.

So keep as-is ?

My understanding of the directory layout guidance in the docs is that different styles get different directories.

Yes, but for "the same check" we use aliases instead. When aliasing checks in different categories it is possible to change default configuration values, so we keep the code in one check and do the rest with options.

trixirt updated this revision to Diff 199358.May 13 2019, 7:42 PM

llvm-header-guard is an alias to bugprone-header-guard.
The style is passed through the option 'GuardStyle'

A regression test was added for llvm-header-guard.
llvm unit tests were modified to the new class and option interface.

aaron.ballman added inline comments.May 17 2019, 6:56 AM
clang-tidy/bugprone/HeaderGuardCheck.cpp
50

I think this should be an else if rather than an else. I'd like to see us diagnose unknown guard styles so that we only accept "llvm" and "" as guard styles; this makes it easier to add new guard styles in the future.

docs/clang-tidy/checks/bugprone-header-guard.rst
11

Should we be documenting the GuardStyle option here as well? We could document "llvm" as the only available option currently and mention the llvm-header-guard check as an alias which enables this style.

test/clang-tidy/bugprone-header-guard.cpp
1–3

Can you add a test for the GuardStyle option? Both when specifying llvm and some random unsupported string.

trixirt updated this revision to Diff 200380.May 20 2019, 5:33 PM

Change the GuardStyle data type from a string to an enum.
Add test cases for explicitly setting valid style.
Document valid style values.
Reduce llvm-header-guard doc to point to bugprone-header-guard.

Latest change addresses most of issues.
Outstanding is adding a test when garbage GuardStyle value is fed into config.
The trial testcase used only CHECK-*-NOT which caused complaining from test harness looking for the CHECK- cases.
Looking for something similar in the other checker tests did not turn up anything.
In general it seems like clang-tidy assumes developers get the string names of the options and their values correct and does not provide a lot of (any?) checking.

Latest change addresses most of issues.
Outstanding is adding a test when garbage GuardStyle value is fed into config.
The trial testcase used only CHECK-*-NOT which caused complaining from test harness looking for the CHECK- cases.
Looking for something similar in the other checker tests did not turn up anything.
In general it seems like clang-tidy assumes developers get the string names of the options and their values correct and does not provide a lot of (any?) checking.

We're probably quite inconsistent in this regard, and it seems like something we may want to address across the board (not suggesting that you have to fix it, or even as part of this patch). I was envisioning the test as being something like CHECK-MESSAGES-ERROR: 1:1: error: %s is an unsupported header guard style or something along those lines.

clang-tidy/bugprone/HeaderGuardCheck.cpp
31

Can you add a newline for some visual separation?

trixirt updated this revision to Diff 200630.May 21 2019, 7:57 PM

Add a newline to separate functions

trixirt marked an inline comment as done.May 21 2019, 8:01 PM
trixirt added inline comments.
clang-tidy/bugprone/HeaderGuardCheck.cpp
31

I did not think ment a empty newline in the param-list, so I assumed you ment the function above and this function could use a new line.

aaron.ballman marked an inline comment as done.May 22 2019, 6:37 AM
aaron.ballman added inline comments.
clang-tidy/bugprone/HeaderGuardCheck.cpp
31

You guessed correctly -- sorry for the lack of clarity. :-)

garymm added a subscriber: garymm.Jul 3 2023, 12:14 PM

@aaron.ballman what's remaining for this to be mergeable?

Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2023, 12:14 PM

@aaron.ballman what's remaining for this to be mergeable?

It needs to be rebased onto trunk and it looks like there's some test coverage missing for unsupported styles. It should probably go through more review with the current code owners as well, in case the landscape in clang-tidy has changed in the past few years in this area.

clang-tidy/llvm/CMakeLists.txt