Page MenuHomePhabricator

[clang-tidy] Add duplicated access specifier readability check (PR25403)
Needs ReviewPublic

Authored by m4tx on Dec 17 2018, 2:39 PM.

Details

Summary

This finds redundant access specifier declarations inside classes, structs, and unions.

https://bugs.llvm.org/show_bug.cgi?id=25403

Diff Detail

Event Timeline

m4tx created this revision.Dec 17 2018, 2:39 PM

Please close PR25403 after commit.

docs/ReleaseNotes.rst
70

Please use alphabetical order for new checks.

m4tx retitled this revision from [clang-tidy] Add duplicated access specifier readability check to [clang-tidy] Add duplicated access specifier readability check (PR25403).Dec 17 2018, 2:55 PM
m4tx edited the summary of this revision. (Show Details)
m4tx added reviewers: alexfh, aaron.ballman, JonasToth.
Eugene.Zelenko added a project: Restricted Project.
m4tx updated this revision to Diff 178539.Dec 17 2018, 2:59 PM

Use alphabetical order in ReleaseNotes.rst

m4tx marked an inline comment as done.Dec 17 2018, 3:00 PM

Please add tests with preprocessor (#if ...) that will show that it ignores disabled code. e.g.:

class ProbablyValid {
private:
  int a;
#if defined(ZZ)
public:
  int b;
#endif
private:
  int c;
protected:
  int d;
public:
  int e;
};
m4tx updated this revision to Diff 178543.Dec 17 2018, 3:29 PM

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

Thanks! CXXRecordDecl is already huge and so adding iterators for a single checker is in my opinion not a good idea (especially if the alternative is actually less code).
Would it make sense to also issue a diagnostic where the first access specifier is redundant (ie public in a struct, and private in a class) ?

Eugene.Zelenko added inline comments.Dec 17 2018, 6:23 PM
clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
36 ↗(On Diff #178543)

Type is not obvious here, so please don't use auto.

aaron.ballman added inline comments.Dec 18 2018, 5:21 AM
clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
21 ↗(On Diff #178543)

You should only register this matcher in C++ mode.

31 ↗(On Diff #178543)

Please switch to const AccessSpecDecl *. Also, that should be LastAccessDecl per the naming conventions.

33 ↗(On Diff #178543)

Why NS -- that seems like a strange naming choice.

36 ↗(On Diff #178543)

Also, decl doesn't match our naming conventions -- change to ASDecl?

m4tx updated this revision to Diff 178777.Dec 18 2018, 1:41 PM

Fix variable names, add macro expansion checking as well as macro and inner class tests.

m4tx marked 6 inline comments as done.Dec 18 2018, 1:42 PM
m4tx added inline comments.
clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
33 ↗(On Diff #178543)

Sorry, this small piece of code was copied from another place and I forgot to change the variable name. I switched to AS, hopefully that's more meaningful here.

m4tx marked an inline comment as done.Dec 18 2018, 1:45 PM

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

Thanks! CXXRecordDecl is already huge and so adding iterators for a single checker is in my opinion not a good idea (especially if the alternative is actually less code).
Would it make sense to also issue a diagnostic where the first access specifier is redundant (ie public in a struct, and private in a class) ?

Yes, I was thinking about the same thing! However, I believe that some people may find this kind of "redundant" access specs actually more readable, so maybe it makes sense to add a check option for this to allow users to disable it?

m4tx added a comment.Dec 18 2018, 1:49 PM

Please add tests with preprocessor (#if ...) that will show that it ignores disabled code. e.g.:

class ProbablyValid {
private:
  int a;
#if defined(ZZ)
public:
  int b;
#endif
private:
  int c;
protected:
  int d;
public:
  int e;
};

Is this actually possible? It seems that macros are ran through the preprocessor before one can fiddle with them in clang-tidy. In other words, int b is not at all present in the AST. However, I added a code to detect macro expansions, so duplicated access specifiers are ignored if at least one of them comes from a macro. If there is a way to cover your case as well, please let me know, because even after looking at the code of other checks I haven't found out a solution for this.

Please add tests with preprocessor (#if ...) that will show that it ignores disabled code. e.g.:

class ProbablyValid {
private:
  int a;
#if defined(ZZ)
public:
  int b;
#endif
private:
  int c;
protected:
  int d;
public:
  int e;
};

Is this actually possible?
It seems that macros are ran through the preprocessor before one can fiddle with them in clang-tidy.
In other words, int b is not at all present in the AST.

.. and by "ignores" i meant that it will be diagnosing this code, since it did not know anything about the code within the preprocessor-disabled section.

However, I added a code to detect macro expansions, so duplicated access specifiers are ignored if at least one of them comes from a macro. If there is a way to cover your case as well, please let me know, because even after looking at the code of other checks I haven't found out a solution for this.

Yes, I was thinking about the same thing! However, I believe that some people may find this kind of "redundant" access specs actually more readable, so maybe it makes sense to add a check option for this > to allow users to disable it?

Perhaps ? I will leave someone with more experience here to comment.

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

Thanks! CXXRecordDecl is already huge and so adding iterators for a single checker is in my opinion not a good idea (especially if the alternative is actually less code).
Would it make sense to also issue a diagnostic where the first access specifier is redundant (ie public in a struct, and private in a class) ?

Yes, I was thinking about the same thing! However, I believe that some people may find this kind of "redundant" access specs actually more readable, so maybe it makes sense to add a check option for this to allow users to disable it?

I'm on the fence about whether this needs to be an option or not. Perhaps having some data on the rate of diagnosis would be helpful here -- can you try adding the feature and running it over some large code bases to see if the implicit access spec warning triggers a lot more than expected? Also, I kind of think this check should be named readability-redundant-access-specifiers instead of duplicated if it's going to consider implicit access specifiers (since the user doesn't write those, "duplicate" may be surprising).

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
43 ↗(On Diff #178777)

Add a full stop at the end of the sentence.

51 ↗(On Diff #178777)

There is no %0 in the diagnostic string, so I'm not certain what this argument is trying to print out. Did you mean ASDecl->getSourceRange() for the underlining?

31 ↗(On Diff #178543)

Still need to switch where the const is.

m4tx marked 4 inline comments as done.Thu, Dec 20, 1:07 PM

Please add tests with preprocessor (#if ...) that will show that it ignores disabled code. e.g.:

class ProbablyValid {
private:
  int a;
#if defined(ZZ)
public:
  int b;
#endif
private:
  int c;
protected:
  int d;
public:
  int e;
};

Is this actually possible?
It seems that macros are ran through the preprocessor before one can fiddle with them in clang-tidy.
In other words, int b is not at all present in the AST.

.. and by "ignores" i meant that it will be diagnosing this code, since it did not know anything about the code within the preprocessor-disabled section.

However, I added a code to detect macro expansions, so duplicated access specifiers are ignored if at least one of them comes from a macro. If there is a way to cover your case as well, please let me know, because even after looking at the code of other checks I haven't found out a solution for this.

Ok, thanks for the clarification. I've added the test in the latest diff!

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
51 ↗(On Diff #178777)

Sorry, this is another line I forgot to remove. Thanks for pointing this out!

By the way, does the underlining work in clang-tidy's diag() function? I see it is used outside the project, but here only FixItHints seem to generate underline in the generated messages.

m4tx updated this revision to Diff 179138.Thu, Dec 20, 1:08 PM
m4tx marked an inline comment as done.
aaron.ballman added inline comments.Fri, Dec 21, 11:57 AM
clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
51 ↗(On Diff #178777)

That's a good question that I don't actually know the answer to. :-D I believe it still works -- you can try it out by passing a SourceRange to the diag() call, like:

diag(LastASDecl->getLocation(), "duplicated access specifier") << SomeSourceRange;

You should see the range from SomeSourceRange underlined.

JonasToth added inline comments.Sun, Dec 23, 4:19 AM
clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp
51 ↗(On Diff #178777)

I believe, if you pass in a token or tokenrange or so, the underlying exists. I thought i got suprised by something like that once, not sure though.

m4tx updated this revision to Diff 181474.Sun, Jan 13, 8:36 AM
m4tx marked an inline comment as not done.

I've added the implicit access specifier check and run it on some bigger codebases. My findings are as below:

  • Dolphin: 6 triggers across 856 record types
  • OpenCV: 31 triggers across 3370 record types
  • Inkscape: 39 triggers across 846 record types
  • LLVM (Core + Clang + Clang Tools Extra): 128 triggers across 9214 record types
  • Blender: 948 triggers across 6264 record types

Since the check was triggered in every big project I've tested, I've decided to add an option (disabled by default) to enable the implicit access specifier check. It is included in my latest Diff, along with the rename to readability-redundant-access-specifiers.