[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

efriedma added a comment. I agree, it doesn't make sense to warn on static functions; the behavior didn't change, and there's only one reasonable result. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67414/new/ https://reviews.llvm.org/D67414

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

smeenai added a comment. In D67414#1873445 , @efriedma wrote: > > https://gcc.godbolt.org/z/cY9-HQ > > gcc's behavior for your testcase makes no sense. We have to emit the > definition of a static function: it can't be defined in any other translation

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

efriedma added a comment. > https://gcc.godbolt.org/z/cY9-HQ gcc's behavior for your testcase makes no sense. We have to emit the definition of a static function: it can't be defined in any other translation unit because it's impossible to name in any other translation unit. Note the "_ZL"

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

smeenai added a comment. It looks like our behavior still differs from gcc in the case of a `static inline __attribute__((gnu_inline))` function: https://gcc.godbolt.org/z/cY9-HQ. We emit it and gcc doesn't. I don't think that combination makes a lot of sense, but I ran into it in some

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

This revision was automatically updated to reflect the committed changes. Closed by commit rL373078: [clang] [AST] Treat inline gnu_inline the same way as extern inline… (authored by mstorsjo, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good to me, thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67414/new/ https://reviews.llvm.org/D67414 ___ cfe-commits

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

mstorsjo updated this revision to Diff 221086. mstorsjo added a comment. Updated the CUDA test based on the suggestion. @rsmith - What do you think of this version, the warning message, and the invariants for the `isInlineDefinitionExternallyVisible` method? CHANGES SINCE LAST ACTION

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

tra added inline comments. Comment at: clang/test/SemaCUDA/gnu-inline.cu:8 void foo(); -inline __attribute__((gnu_inline)) void bar() { foo(); } +inline __attribute__((gnu_inline)) void bar() { foo(); } // expected-warning {{'gnu_inline' attribute without 'extern' in C++

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

mstorsjo marked an inline comment as done. mstorsjo added inline comments. Comment at: clang/test/SemaCUDA/gnu-inline.cu:8 void foo(); -inline __attribute__((gnu_inline)) void bar() { foo(); } +inline __attribute__((gnu_inline)) void bar() { foo(); } // expected-warning

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

mstorsjo marked an inline comment as done. mstorsjo added inline comments. Comment at: clang/lib/AST/Decl.cpp:3334 /// For an inline function definition in C, or for a gnu_inline function /// in C++, determine whether the definition will be externally visible. ///

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

mstorsjo updated this revision to Diff 220053. mstorsjo added a comment. Adapted based on the feedback so far, suggestions on naming and grouping the warning are welcome. The warning did trigger in an existing CUDA test as well - I'm not familiar with cuda and how it relates to other

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

mstorsjo added a comment. In D67414#1668579 , @rsmith wrote: > OK. If this is causing compatibility pain in practice, following GCC here > seems reasonable. We should at least produce a warning on `gnu_inline` > without `extern` in C++ mode, though.

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

rsmith added a comment. In D67414#1668475 , @mstorsjo wrote: > In D67414#1668443 , @rsmith wrote: > > > Seems very surprising to me that the `gnu_inline` attribute has different > > behavior in their C and C++

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

mstorsjo added a comment. In D67414#1668443 , @rsmith wrote: > Seems very surprising to me that the `gnu_inline` attribute has different > behavior in their C and C++ frontends. That said, it looks like it's a > behavior that they document; their

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

rsmith added a comment. Seems very surprising to me that the `gnu_inline` attribute has different behavior in their C and C++ frontends. That said, it looks like it's a behavior that they document; their documentation says "In C++, this attribute does not depend on extern in any way, but it

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

mstorsjo marked 2 inline comments as done. mstorsjo added inline comments. Comment at: lib/AST/Decl.cpp:3283 if (Context.getLangOpts().CPlusPlus) return false; nickdesaulniers wrote: > I would have thought the existing case here would handle your

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

nickdesaulniers added inline comments. Comment at: lib/AST/Decl.cpp:3283 if (Context.getLangOpts().CPlusPlus) return false; I would have thought the existing case here would handle your change. If it doesn't, why not? Should your change also remove

[PATCH] D67414: [AST] Treat "inline gnu_inline" the same way as "extern inline gnu_inline" in C++ mode

mstorsjo created this revision. mstorsjo added reviewers: dblaikie, pcc, efriedma. Herald added a project: clang. This matches how GCC handles it, see e.g. https://gcc.godbolt.org/z/HPplnl. The previous behaviour of gnu_inline in C++, without the extern keyword, can be traced back to the