[PATCH] D59402: Suggestions to fix -Wmissing-{prototypes,variable-declarations}

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: lib/Sema/SemaDecl.cpp:13345-13346
+            << (FD->getStorageClass() == SC_None
+                    ? FixItHint::CreateInsertion(FD->getTypeSpecStartLoc(),
+                                                 "static ")
+                    : FixItHint{});
----------------
aaron.ballman wrote:
> aaronpuchert wrote:
> > aaron.ballman wrote:
> > > We may not want to produce the fixit if there's a macro involved in the declaration. Consider:
> > > ```
> > > #ifdef SOMETHING
> > > #define FROBBLE static
> > > #else
> > > #define FROBBLE
> > > #endif
> > > 
> > > FROBBLE void foo(void);
> > > ```
> > > We probably don't want the fixit in the case `SOMETHING` is not defined.
> > I think that's generally an issue with fix-its, there could always be a macro that turns the code into something entirely different. If we look at the other fix-it above, we can construct
> > 
> > ```
> > #define VOID
> > int f(VOID);
> > int f() { return 0; }
> > ```
> > 
> > Then we get:
> > ```
> > <stdin>:3:5: warning: no previous prototype for function 'f' [-Wmissing-prototypes]
> > int f() { return 0; }
> >     ^
> > <stdin>:2:5: note: this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function
> > int f(VOID);
> >     ^
> >           void
> > ```
> > 
> > Then the fix-it doesn't even work in the original configuration, because it produces `int f(VOIDvoid)`. If we make it work by adding a space, we still have the problem that you mentioned: if someone defines `VOID` as `void`, we then have `int f(void void)` after applying the fix-it in the original setting.
> > 
> > Trying to make fix-its work with macros is probably a hopeless endeavor.
> > Trying to make fix-its work with macros is probably a hopeless endeavor.
> 
> That's what I was getting at -- I think you need to see if there's a macro at the insertion location and not generate a fix-it in that case. The above suffers from the same issue (and can be corrected in a subsequent patch, if desired).
How can I know that? The AST seems to contain no such information, for the example I only see the following:

```
TranslationUnitDecl 0x12574e8 <<invalid sloc>> <invalid sloc>
|-[...]
|-FunctionDecl 0x12b5340 <<stdin>:2:1, col:11> col:5 f 'int ()'
`-FunctionDecl 0x12b5440 prev 0x12b5340 <line:3:1, col:21> col:5 f 'int ()'
  `-CompoundStmt 0x12b5508 <col:9, col:21>
    `-ReturnStmt 0x12b54f8 <col:11, col:18>
      `-IntegerLiteral 0x12b54d8 <col:18> 'int' 0
```

It seems fix-its are automatically suppressed if they would land in a macro definition, like

```
#define X int f()
X;
int f() { return 0; }
```

produces

```
<stdin>:3:5: warning: no previous prototype for function 'f' [-Wmissing-prototypes]
int f() { return 0; }
    ^
<stdin>:2:1: note: this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function
X;
^
<stdin>:1:15: note: expanded from macro 'X'
#define X int f()
              ^
```

So there is no fix-it suggested, although we produce one. I guess some intermediate layer that knows about the connection to the original source detects that and throws it away.

But what you want is that we also suppress it if there is any macro (it wouldn't need to be at the beginning, it could also be `void FROBBLE foo(void)` with the same meaning). That would also mean I can't apply the fix-it even if the macros is unrelated, say if someone has

```
#define ATTR_PURE __attribute__((pure))
ATTR_PURE int f() { return 0; }
```

Other fix-its (that I have looked at) just seem to ignore macros, like we do, relying on fix-its to be discarded if they would land inside of a macro.

I think there is a case to be made for avoiding `int f(VOIDvoid)`, but not suggesting the fix-it because a macro could have another meaning in another configuration seems unusual to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59402