[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

kimgr added a comment. @dblaikie I think @sammccall mentioned `IWYU pragma: friend` before, that's what I was alluding to. Much of IWYU's complexity comes from maintaining these library mappings both statically (using mapping tables) and dynamically (using in-source annotations). I figured the

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

rsmith added a comment. In D106394#2923186 , @dblaikie wrote: > In D106394#2922972 , @rsmith wrote: > >> ... As an alternative, have you considered checking whether any of the >> headers named in the pragma are i

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

dblaikie added a comment. In D106394#2922972 , @rsmith wrote: > ... As an alternative, have you considered checking whether any of the > headers named in the pragma are in the include stack, and warning if not? > That would seem to make this warning app

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

rsmith added inline comments. Comment at: clang/lib/Lex/Pragma.cpp:1996 AddPragmaHandler("clang", new PragmaSystemHeaderHandler()); + AddPragmaHandler("clang", new PragmaIncludeInsteadHandler()); AddPragmaHandler("clang", new PragmaDebugHandler()); Quuxplu

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

rsmith added a comment. In D106394#2923007 , @cjdb wrote: > In D106394#2922972 , @rsmith wrote: > >> As an alternative, have you considered checking whether any of the headers >> named in the pragma are in the in

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

Quuxplusone added inline comments. Comment at: clang/lib/Lex/Pragma.cpp:1996 AddPragmaHandler("clang", new PragmaSystemHeaderHandler()); + AddPragmaHandler("clang", new PragmaIncludeInsteadHandler()); AddPragmaHandler("clang", new PragmaDebugHandler()); @r

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb added a comment. In D106394#2922972 , @rsmith wrote: > I'm not in love with the design of this feature. Basing the accept / warn > decision on whether the inclusion is in a system header seems fragile and > doesn't seem to enforce the intended sema

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

rsmith added a comment. I'm not in love with the design of this feature. Basing the accept / warn decision on whether the inclusion is in a system header seems fragile and doesn't seem to enforce the intended semantics of the warning (that you can only reach the implementation detail header thr

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

dblaikie added a comment. In D106394#2921830 , @kimgr wrote: > Took me a while to get my head around it, but I see now that this is only > supported for system headers. I think that makes sense for the compiler, > otherwise it will be hard to guess whic

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

kimgr added a comment. Hi, sorry I'm late to the game. IWYU maintainer here. I saw this patch mentioned in the LLVM Weekly newsletter and immediately thought: "wow, great, I have to build support in IWYU for that!". I too prefer pragmas to magic comments, and I don't think `include_instead` ne

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

dblaikie added a comment. In D106394#2910571 , @cjdb wrote: > In D106394#2905660 , @dblaikie > wrote: > >> Just a thought (nothing to hold up this patch/suggest a revert/suggest any >> immediate action), but: >>

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb added a comment. In D106394#2905660 , @dblaikie wrote: > Just a thought (nothing to hold up this patch/suggest a revert/suggest any > immediate action), but: > >> The problem with extending this to non-system headers is that you need a way >> to te

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

hans added a comment. In D106394#2907520 , @hans wrote: > This is causing the compiler to crash in Chromium builds, see > https://bugs.chromium.org/p/chromium/issues/detail?id=1195353 Durr, that should be https://bugs.chromium.org/p/chromium/issues/det

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

hans added a comment. This is causing the compiler to crash in Chromium builds, see https://bugs.chromium.org/p/chromium/issues/detail?id=1195353 I verified that replacing the use of getLiteralData() with OriginalFileName in HandleHeaderIncludeOrImport() fixes the crash, but the the other getL

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

dblaikie added a comment. Just a thought (nothing to hold up this patch/suggest a revert/suggest any immediate action), but: > The problem with extending this to non-system headers is that you need a way > to tell which headers are allowed to include the detail headers and which > ones are not

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe8a64e549126: [clang][pp] adds '#pragma include_instead' (authored by cjdb). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:304 + +def pp_pragma_include_instead_not_sysheader : Error< + "'#pragma clang include_instead

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb added inline comments. Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:304 + +def pp_pragma_include_instead_not_sysheader : Error< + "'#pragma clang include_instead' cannot be used outside of system headers">, aaron.ballman wrote: > Can you pref

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb updated this revision to Diff 361357. cjdb marked 11 inline comments as done. cjdb added a comment. applies all of @aaron.ballman's remaining feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106394/new/ https://reviews.llvm.org/D106394

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:304 + +def pp_pragma_include_instead_not_sysheader : Error< + "'#pragma clang include_instead' cannot be used outside of system headers">, Can you prefix the diagnos

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb updated this revision to Diff 361014. cjdb marked 12 inline comments as done. cjdb added a comment. - applies all of @aaron.ballman's suggested changes - turns warnings into errors and deletes warning group - replaces `interleave` with `join` - properly abandons LF transmuting The only thing

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb marked 2 inline comments as done. cjdb added inline comments. Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:307-310 +def pp_pragma_include_instead_unexpected_token : Warning< + "'#pragma include_instead' expects '%0' as its next token; got '%1' instead">, +

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb added a comment. Thanks for the first pass @aaron.ballman! Some clarifying questions, but I think I have enough of your other comments to work on while you ponder these. Comment at: clang/include/clang/Basic/DiagnosticLexKinds.td:303 InGroup>; +def pp_pragma_include_in

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

aaron.ballman added a comment. Pragmas and magic comments both have ergonomic issues, so it's really about the tradeoffs. Personally, I think it's more reasonable to support this as a pragma than as a magic-comment. Pragmas are a mystery to users, but they're less of a mystery than comments tha

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

sammccall added a comment. In D106394#2893457 , @ldionne wrote: > In D106394#2892832 , @sammccall > wrote: > >> Eventually this seems like a reasonable thing to want for user code. What >> are your motivations f

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb added a comment. In D106394#2893681 , @Quuxplusone wrote: > If IWYU has already developed a de facto standard for this information, I > would recommend libc++ just use it (perhaps with no changes needed in clang > at all). This would have the bene

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb updated this revision to Diff 360519. cjdb added a comment. - renames files - makes it possible for `#pragma GCC system_header` to come after includes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106394/new/ https://reviews.llvm.org/D106394

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

Quuxplusone added a comment. > Hmm... I like prior art. That clangd supports it suggests that there's a > section of code I can look at for inspiration if we were to replace this > pragma with the IWYU comment-pragma (I wonder why they didn't just go with > `#pragma IWYU ...`?). Is it reasonabl

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb added a comment. In D106394#2892832 , @sammccall wrote: > This is pretty interesting from a tooling perspective! > > Some prior art here are the include-what-you-use "pragmas" >

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

ldionne added a comment. This is quite interesting, I'd love to be able to use this in libc++ (and suggest that my peers use this in Apple's libc)! In D106394#2892832 , @sammccall wrote: > Eventually this seems like a reasonable thing to want for user c

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

sammccall added a comment. This is pretty interesting from a tooling perspective! Some prior art here are the include-what-you-use "pragmas" (magic comments). Clangd supports `// IWYU pragma: "private

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb updated this revision to Diff 360317. cjdb marked an inline comment as done. cjdb added a comment. - renames test files so they better describe intention - replaces `SmallVector` with `SetVector` - replaces `std::accumulate` with `llvm::interleave` Repository: rG LLVM Github Monorepo CHA

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb added a comment. In D106394#2891647 , @zoecarver wrote: > Can't wait to start using this! (Note: this is not a full review, just some > thoughts.) > > Do you have a test case where > > // private: header one > *exists* > > // private: heade

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

zoecarver added a comment. Can't wait to start using this! (Note: this is not a full review, just some thoughts.) Do you have a test case where // private: header one *exists* // private: header two #include<__header_one.h> // public: header three #include<__header_two.h> I t

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb added inline comments. Comment at: clang/lib/Lex/PPLexerChange.cpp:437 + diag::pp_pragma_include_instead_system_reserved) +<< Filename << 2 << ('{' + Aliases + '}'); + } I wasn't able to work out how to escape braces inside diagn

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

cjdb created this revision. cjdb added reviewers: aaron.ballman, rsmith, gbiv, dblaikie, cor3ntin. Herald added a reviewer: george.burgess.iv. Herald added subscribers: usaxena95, kadircet. cjdb requested review of this revision. Herald added subscribers: cfe-commits, ilya-biryukov. Herald added a