[PATCH] D55793: [clang-tidy] Add duplicated access specifier readability check (PR25403)

[Mateusz Maćkowski via Phabricator via cfe-commits]

m4tx marked 4 inline comments as done.
m4tx added a comment.

In D55793#1335274 <https://reviews.llvm.org/D55793#1335274>, @lebedev.ri wrote:

> In D55793#1335249 <https://reviews.llvm.org/D55793#1335249>, @m4tx wrote:
>
> > In D55793#1333661 <https://reviews.llvm.org/D55793#1333661>, @lebedev.ri wrote:
> >
> > > 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!



================
Comment at: clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp:51
+      diag(ASDecl->getLocation(), "duplicated access specifier")
+          << MatchedDecl
+          << FixItHint::CreateRemoval(ASDecl->getSourceRange());
----------------
aaron.ballman wrote:
> 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?
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 `FixItHint`s seem to generate underline in the generated messages.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55793/new/

https://reviews.llvm.org/D55793