[PATCH] D107095: Implement #pragma clang header_unsafe

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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:
----------------
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`?


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1309
+
+// Warnings and extensions to make preprocessor macro usage pedantic
+def PedanticMacros : DiagGroup<"pedantic-macros",
----------------
Speaking of making things pedantic... :-D


================
Comment at: clang/include/clang/Basic/IdentifierTable.h:127
 
-  // 24 bits left in a 64-bit word.
+  // True if this macro is unsafe in headers
+  unsigned IsHeaderUnsafe : 1;
----------------



================
Comment at: clang/include/clang/Basic/IdentifierTable.h:192-193
     } else {
+      // Since calling the setters of these calls recompute, just set them
+      // manually to avoid calling recompute a bunch of times.
+      IsDeprecatedMacro = false;
----------------



================
Comment at: clang/include/clang/Lex/Preprocessor.h:798
 
+  /// Usage warning for macros marked by #pragma clang header_unsafe
+  llvm::DenseMap<const IdentifierInfo *, std::string> HeaderUnsafeMacroMsgs;
----------------



================
Comment at: clang/include/clang/Lex/Preprocessor.h:2414
+  void addHeaderUnsafeMsg(const IdentifierInfo *II, std::string Msg) {
+    HeaderUnsafeMacroMsgs.insert(std::make_pair(II, Msg));
+  }
----------------
Is this going to cause a copy for `Msg` without a call to `move()`?


================
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)
----------------
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?


================
Comment at: clang/test/Lexer/Inputs/unsafe-macro-2.h:23-26
+// not-expected-warning at +1{{macro 'UNSAFE_MACRO_2' has been marked as unsafe for use in headers}}
+#undef UNSAFE_MACRO_2
+// not-expected-warning at +1{{macro 'UNSAFE_MACRO_2' has been marked as unsafe for use in headers}}
+#define UNSAFE_MACRO_2 2
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107095