dexonsmith accepted this revision. dexonsmith added a comment. This revision is now accepted and ready to land.
LGTM, although I have a suggested change for one of the comments (and I'm still sad about this). ================ Comment at: clang/lib/Lex/DependencyDirectivesSourceMinimizer.cpp:766-769 + // If "#ifdef" is empty, strip it and skip the "#endif". + // Avoid stripping "#if" / "#elif" as they could contain a "__has_include" + // (either directly, or through a macro expansion) without an "#include" + // inside the "#if/elif", which could affect the dependency output. ---------------- I'd prefer to structure this comment differently, something like: ``` // If "#ifdef" is empty, strip it and skip the "#endif". // // TODO: Once/if Clang starts disallowing __has_include in macro expansions, // we can skip empty `#if` and `#elif` blocks as well after scanning for a // literal __has_include in the condition. Even without that rule we could // drop the tokens if we scan for identifiers in the condition and find none. ``` This rephrases it as a potential future optimization instead of one that has to be skipped, which I think makes it easier to reason about. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70936/new/ https://reviews.llvm.org/D70936 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
