aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp:51
this);
- Finder->addMatcher(typedefNameDecl().bind(TypedefId), this);
+ Finder->addMatcher(typedefNameDecl(unless(isImplicit())).bind(TypedefId),
this);
auto ParenFunctionType = parenType(innerType(functionType()));
----------------
Might as well fix the clang-format issue.
================
Comment at: clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp:149
Token ProtoToken;
+ IdentifierTable &Idents = Result.Context->Idents;
+ int MacroLevel = 0;
----------------
I think a safer interface to this is to use a `const IdentifierTable &` here
and in `isMacroIdentifier()`. Then, in `isMacroIdentifier()`, instead of using
`get()` (which has no `const` overload because it can add an identifier to the
table), you can use `find()`. This removes any possibility of accidentally
adding identifiers to the table (even in future refactorings).
================
Comment at:
clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp:157-161
+ if (ProtoToken.is(tok::TokenKind::l_paren)) {
+ State = TokenState::LeftParen;
+ } else if (isMacroIdentifier(Idents, ProtoToken)) {
+ State = TokenState::MacroId;
+ }
----------------
================
Comment at:
clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp:164-168
if (ProtoToken.is(tok::TokenKind::l_paren)) {
- State = SawLeftParen;
+ State = TokenState::MacroLeftParen;
+ } else {
+ State = TokenState::Start;
+ }
----------------
================
Comment at:
clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp:173-177
+ if (isMacroIdentifier(Idents, ProtoToken)) {
+ State = TokenState::MacroId;
+ } else {
+ State = TokenState::MacroArguments;
+ }
----------------
================
Comment at:
clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp:180-184
+ if (MacroLevel == 0) {
+ State = TokenState::Start;
+ } else {
+ State = TokenState::MacroId;
+ }
----------------
================
Comment at:
clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp:215-219
if (ProtoToken.is(tok::TokenKind::r_paren)) {
removeVoidToken(VoidToken, Diagnostic);
} else if (ProtoToken.is(tok::TokenKind::l_paren)) {
- State = SawLeftParen;
+ State = TokenState::LeftParen;
}
----------------
================
Comment at:
clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp:224-226
+ if (State == TokenState::Void && ProtoToken.is(tok::TokenKind::r_paren)) {
removeVoidToken(VoidToken, Diagnostic);
}
----------------
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp:202
// intentionally not LLVM style to check preservation of whitespace
+// clang-format off
typedef
----------------
Unrelated change can be split out an NFC commit if you'd like, but we don't
typically attempt to clang-format existing test files, so also not really
necessary either.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp:561
+#define return_t(T) T
+return_t(void) func(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in
function declaration
----------------
LegalizeAdulthood wrote:
> jrtc27 wrote:
> > LegalizeAdulthood wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > LegalizeAdulthood wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Can you also add a test for:
> > > > > > > ```
> > > > > > > void func(return_t(void));
> > > > > > > ```
> > > > > > `:-)`
> > > > > >
> > > > > > What are you suggesting the result should be? Honestly, looking at
> > > > > > that, I'm not sure myself `:)`
> > > > > >
> > > > > > IMO, if I saw this in a code review, I would flag it because you're
> > > > > > using a macro called "return type" to specify the type of an
> > > > > > argument.
> > > > > LoL, yeah, the name `return_t` would certainly be novel to use in a
> > > > > parameter list, but what I was hoping to test is whether we try to
> > > > > fix the use of the macro within the parameter list or not. I *think*
> > > > > it probably makes sense to issue the diagnostic, but I don't think it
> > > > > makes sense to try to fix it because the macro could be defined
> > > > > differently for different configurations. But the diagnostic is
> > > > > silenced as well as the fix-it, I wouldn't lose a whole lot of sleep
> > > > > over it.
> > > > Well it could conceivably be used to declare a function pointer
> > > > argument like this:
> > > >
> > > > `void func(return_t(void) (*fp)(void));`
> > > >
> > > > In that case, my expectation is that the check would fix the void arg,
> > > > but not the arg to the macro.
> > > OK, that was a good idea to add the test I described above because it
> > > failed `:)`,
> > > so let me improve the check some more.
> > If you want a less-contrived example that shows up all over the place in
> > crusty old C code that supports (or, perhaps, supported and let bitrot
> > support for) pre-ANSI C compilers:
> >
> > ```
> > #define __P(x) x
> > void foo __P((void));
> > ```
> >
> > (the idea being that, for pre-ANSI C compilers, you'd instead define __P(x)
> > as () to get `void foo ();`)
> Well, in (modern) C you don't want to remove the `void` in `(void)` at all.
>
> This check applies only to C++ code, so we should be safe here.
>
> I'll add a test case just to cover it -- I think the correct result should be
> that the check leaves this instance of `(void)` intact.
> Well, in (modern) C you don't want to remove the void in (void) at all.
In everything but `-ansi` mode, in fact (C89 supported prototypes). :-D
> I'll add a test case just to cover it -- I think the correct result should be
> that the check leaves this instance of (void) intact.
Thanks for the coverage, and I agree, we shouldn't try mucking with a fix in
the presence of a macro.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116425/new/
https://reviews.llvm.org/D116425
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
