[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:3916
+processed by the preprocessor after the ``#pragma`` annotation will log a
+a warning. For example:
+
----------------
May want to put something in here along the lines of: Redefining the macro or undefining the macro will not be diagnosed, nor will expansion of the macro within the main source file.


================
Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:531
+          "%select{|: %2}1">,
+  InGroup<PedanticMacros>;
+
----------------
Should this get its own warning flag within the pedantic macros group? (Otherwise, it's awkward to try to silence this diagnostic while retaining some of the other ones like deprecated pragma.)


================
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
----------------
beanz wrote:
> aaron.ballman wrote:
> > beanz wrote:
> > > aaron.ballman wrote:
> > > > beanz wrote:
> > > > > 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
> > > > So `header_unsafe` is "diagnose if someone expands this macro from outside the main source file" and `final` is "diagnose if someone defines or undefines this macro anywhere", correct? Would it make sense to have a shorthand to combine these effects for a "fully reserved" macro identifier (`#pragma clang reserve_macro(IDENT[, msg])` as a strawman)?
> > > My thought process for implementing them separately was that final would be useful independent of header_unsafe. I could, for example, see applying final to macros like MIN and MAX, where they can be safely used anywhere, but you really don’t want multiple definitions floating around. 
> > FWIW, I agree that having separation is useful -- I think these serve orthogonal (but related) purposes: macros which can only be used by "user code", and macros which cannot be redefined or undefined. I was thinking that would be an additional pragma instead of a replacement for the two proposed.
> > 
> > I should probably tell you my use cases so we're both on the same page. One of the most frustrating problems with trying to write a highly portable library is the fact that I have to worry about users defining macros that may conflict with my identifiers (like function names, structure names, template names, etc), but I have no way to reserve those identifiers. I'm hopeful we can find a way that I can "protect" those identifiers in a library with an extension that basically says "you can't use these identifiers for macro purposes without breaking me". I think that's a combination of `header_unsafe` and `final` -- I don't want other libraries to start using macros with the names of my library's functions if my header has been included somewhere, and I don't want user code defining macros that may conflict with my library.
> Ah! That makes perfect sense. I think I misunderstood your comment. Totally agree, having a shorthand that groups both together would be super useful. I suspect we'll have some uses where we will want both `final` and `header_unsafe` too!
Excellent! FWIW, I don't think the combined pragma has any impact on the behavior of this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107095