Page MenuHomePhabricator

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

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 ↗(On Diff #178536)

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.Dec 20 2018, 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.Dec 20 2018, 1:08 PM
m4tx marked an inline comment as done.
aaron.ballman added inline comments.Dec 21 2018, 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.Dec 23 2018, 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.Jan 13 2019, 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.

m4tx updated this revision to Diff 195408.Apr 16 2019, 10:03 AM

Updated the diff for the latest (master) version, used GitHub monorepo for the changes

Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 10:03 AM
clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
35

Please use double back-ticks for language keywords, like private anc class.

48

Please use single back-ticks for option names.

m4tx updated this revision to Diff 195409.Apr 16 2019, 10:18 AM

Updated the backticks in the documentation.

aaron.ballman added inline comments.Apr 22 2019, 5:12 AM
clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp
35–36

I have a slight preference for using assignment operators here rather than explicit constructor calls.

54

This is a bit terse, how about: redundant access specifier has the same accessibility as the implicit access specifier?

69

This is a bit terse, how about: redundant access specifier has the same accessibility as the previous access specifier?

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
6

structs and unions -> structs, and unions

One thing the docs leave unclear is which access specifiers you're talking about. Currently, the check only cares about the access specifiers for fields, but it seems reasonable that the check could also be talking about access specifiers on base class specifiers. e.g.,

struct Base {
  int a, b;
};

class C : private Base { // The 'private' here is redundant.
};

You should probably clarify this in the docs. Implementing this functionality may or may not be useful, but if you want to implement it, you could do it in a follow-up patch.

33

give warning -> diagnose

34

May also want to put public inside struct or union as well.

48

since -> because

clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
71–72

I think that diagnosing here is unfortunate. If the user defines ZZ, then the access specifier is no longer redundant. However, it may not be easy for you to handle this case when ZZ is not defined because the access specifier will have been removed by the preprocessor.

m4tx updated this revision to Diff 197025.Apr 28 2019, 6:37 AM

Updated per @aaron.ballman comments

m4tx marked 15 inline comments as done.Apr 28 2019, 6:48 AM
m4tx added inline comments.
clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp
35–36

This is not possible here as specific_decl_iterator has the copy constructor marked as explicit.

54

Sounds good, changed this in the latest revision.

69

As above.

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst
6

This is actually included in the documentation (*member* access specifier), but I've added explicit "field and method" clarification.

clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp
71–72

I agree, but I think that we cannot do anything reasonable here. The code is removed by the preprocessor, and I believe the only behavior that would make sense would be to completely suppress the warnings if there is a preprocessor directive between the last access specifier and the current one. However, if I'm not mistaken, there is no way in clang-tidy to detect that.

m4tx updated this revision to Diff 197028.Apr 28 2019, 6:51 AM
m4tx marked 5 inline comments as done.

Remove the accidentally added patch file

This revision is now accepted and ready to land.May 7 2019, 9:38 AM

Given this revision is accepted and seems ready to go: Do you need someone to commit on your behalf?

m4tx added a comment.Jun 10 2019, 12:27 AM

@JonasToth thanks for asking! Yes, since I do not have commit access, I need someone to do the commit for me.

@JonasToth thanks for asking! Yes, since I do not have commit access, I need someone to do the commit for me.

I'm sorry for the terribly long delay on getting this committed -- it fell off my radar for a bit. When I try to apply the branch to trunk, I get merge conflicts. Can you rebase it on ToT, please?