Page MenuHomePhabricator

[clang-tidy] check for using declaration qualification
Needs ReviewPublic

Authored by Naysh on Dec 5 2018, 4:39 PM.

Details

Summary

This patch adds a "Qualified Aliases" check, based off the guidelines in https://abseil.io/tips/119, to the abseil module.

The check flags any using declarations which are not fully qualified.

Diff Detail

Event Timeline

Naysh created this revision.Dec 5 2018, 4:39 PM
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added inline comments.
clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp
31 ↗(On Diff #176900)

Please remove unnecessary empty line.

33 ↗(On Diff #176900)

Please remove unnecessary empty line.

34 ↗(On Diff #176900)

const auto *

clang-tidy/abseil/QualifiedAliasesCheck.cpp
14

Please remove unnecessary empty line.

clang-tidy/abseil/SafelyScopedCheck.cpp
27 ↗(On Diff #176900)

Please run Clang-format over patch.

docs/ReleaseNotes.rst
73

Please enclose using in ``. Same in other places, including documentation.

docs/clang-tidy/checks/abseil-alias-free-headers.rst
10 ↗(On Diff #176900)

Are there guidelines?

docs/clang-tidy/checks/abseil-qualified-aliases.rst
25

Please remove unnecessary empty line.

docs/clang-tidy/checks/abseil-safely-scoped.rst
28 ↗(On Diff #176900)

Please remove unnecessary empty line.

test/clang-tidy/abseil-qualified-aliases.cpp
28

Please remove unnecessary empty line.

test/clang-tidy/abseil-safely-scoped.cpp
27 ↗(On Diff #176900)

Please remove unnecessary empty line.

Eugene.Zelenko added inline comments.Dec 5 2018, 7:22 PM
docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst
20 ↗(On Diff #176900)

Are there guidelines?

docs/clang-tidy/checks/abseil-qualified-aliases.rst
23

Please make text consistent across all these checks.

alexfh requested changes to this revision.Dec 6 2018, 6:00 AM

Could you send each check in a separate patch, please? This makes the review much more straightforward and I'm sure it will be faster too.

This revision now requires changes to proceed.Dec 6 2018, 6:00 AM
alexfh added inline comments.Dec 6 2018, 6:01 AM
clang-tidy/abseil/AbseilTidyModule.cpp
25

Please upload the patch with full context. See https://llvm.org/docs/Phabricator.html

Naysh updated this revision to Diff 177116.EditedDec 6 2018, 9:20 PM
Naysh retitled this revision from [clang-tidy] check for using declaration scope and qualification to [clang-tidy] check for using declaration qualification.
Naysh edited the summary of this revision. (Show Details)

To respond to alexfh's request to split this submission into four separate patches (corresponding to each of the introduced checks), I've updated the patch to only include the "Qualified Aliases" check.

I'll add the other checks as separate patches.

Naysh edited the summary of this revision. (Show Details)Dec 6 2018, 9:32 PM
aaron.ballman added inline comments.Dec 10 2018, 4:47 AM
clang-tidy/abseil/QualifiedAliasesCheck.cpp
29

Drop the top-level const qualifier (here and elsewhere).

34

No need for the assignment.

39

This is missing a check for whether the referenced name is within the current namespace.

43–44

Do not put HTML links into the diagnostic -- the wording should stand on its own. I would probably phrase it "using declaration should use a fully qualified name" or something along those lines.

clang-tidy/abseil/QualifiedAliasesCheck.h
19

This should be fixed.

test/clang-tidy/abseil-qualified-aliases.cpp
22–23

I don't think this should warn, per the tip.

Naysh marked 4 inline comments as done.Dec 10 2018, 5:25 PM
JonasToth resigned from this revision.Dec 30 2019, 7:59 AM