beanz marked 5 inline comments as done.
beanz added a comment.
@aaron.ballman, thanks for the feedback!
Some comments, but I'll work on updated patches and try to get them up tonight
or tomorrow morning.
================
Comment at: clang/docs/LanguageExtensions.rst:3913
+Clang supports the pragma ``#pragma clang header_unsafe``, which can be used to
+mark macros as unsafe to use in headers. This can be valuable when providing
+headers with ABI stability requirements. For example:
----------------
aaron.ballman wrote:
> I think it should be made more clear as to whether this "unsafe to use in
> headers" means "unsafe to use in the header declaring the attribute as
> unsafe" vs "unsafe to use in any header included by the one marked as
> unsafe.", etc.
>
> There's also the rather interesting example of:
> ```
> // foo.h
> #define FROBBLE 1
> #pragma clang header_unsafe(FROBBLE, "why would you ever frobble things?")
>
> // bar.h
> #if 1
> #elifdef FROBBLE
> #endif
>
> // source.c
> #include "foo.h"
> #include "bar.h"
> ```
> Do we diagnose use of FROBBLE in bar.h despite it not directly including
> foo.h? Do we diagnose even though it's in a branch that's not possible to
> take? Does/should the answer change if we swap the order of includes? What if
> we change `#if 1` into something that's actually variable like `FOO > 8`?
My intent was that once marked unsafe, it would warn for _all_ uses in files
that aren't the main source file.
In your example, the warning wouldn't fire because the preprocessor doesn't
expand skipped block conditionals, but it would if the block wasn't skipped.
It also is order-based, so the warning only fires on uses after the pragma. My
thought was that could be a way to allow the header defining the macro to use
it in some limited set of expansions. i.e:
```
// foo.h
//We only frobble on arm...
#if __is_target_arch(arm)
#define FROBBLE 1
#endif
#if FROBBLE
...
#endif
#pragma clang header_unsafe(FROBBLE, "Nobody else gets to Frobble in headers
but me!")
```
Then in any user code, including `foo.h` causes FROBBLE to warn in all
subsequent included sources.
The concrete use case for this is actually macOS Catalyst. Apple provides
TargetConditionals.h which defines `TARGET_OS_*` macros for each target
operating system.
The problem is, with Catalyst, you might be building a codebase that is going
to run on macOS, but needs to be API & ABI compatible with iOS. So we want the
ability to warn on those macros being used in headers, and instead encourage
people to use API availability markup.
================
Comment at: clang/lib/Lex/Preprocessor.cpp:1416-1434
+void Preprocessor::emitMacroDeprecationWarning(const Token &Identifier) {
+ auto DepMsg = getMacroDeprecationMsg(Identifier.getIdentifierInfo());
+ if (!DepMsg)
+ Diag(Identifier, diag::warn_pragma_deprecated_macro_use)
+ << Identifier.getIdentifierInfo() << 0;
+ else
+ Diag(Identifier, diag::warn_pragma_deprecated_macro_use)
----------------
aaron.ballman wrote:
> I think it'd make sense to have a "macro marked <whatever> here" note in both
> of these pragmas (the deprecated one can be handled separately as a later
> thing), WDYT?
+1
Will work on that.
================
Comment at: clang/test/Lexer/Inputs/unsafe-macro-2.h:23-26
+// not-expected-warning@+1{{macro 'UNSAFE_MACRO_2' has been marked as unsafe
for use in headers}}
+#undef UNSAFE_MACRO_2
+// not-expected-warning@+1{{macro 'UNSAFE_MACRO_2' has been marked as unsafe
for use in headers}}
+#define UNSAFE_MACRO_2 2
----------------
aaron.ballman wrote:
> Why do we not expect warnings for these cases? I would have expected that
> undefining a macro is just as unsafe for ABI reasons as defining a macro is.
I kinda waffled on this myself. My thought was to treat this similarly to how
we handle the macro redefinition warning. If you `undef`, you're kind of
claiming the macro as your own and all bets are off...
That said, my next clang extension closes that loop hole too:
https://github.com/llvm-beanz/llvm-project/commit/f0a5216e18f5ee0883039095169bd380295b1de0
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107095/new/
https://reviews.llvm.org/D107095
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
