aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM!
================
Comment at: lib/Sema/SemaDecl.cpp:13345-13346
+ << (FD->getStorageClass() == SC_None
+ ? FixItHint::CreateInsertion(FD->getTypeSpecStartLoc(),
+ "static ")
+ : FixItHint{});
----------------
aaronpuchert wrote:
> 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.
Okay, good point about `void FOO blah(void);` being just as problematic. I was
thinking you would look at `FD->getTypeSpecStartLoc().isMacroID()`, but that
would be insufficient and may be handled by an intermediate layer anyway.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59402/new/
https://reviews.llvm.org/D59402
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
