awarzynski accepted this revision. awarzynski added a comment. This revision is now accepted and ready to land.
While quite extensive, I find the overall logic in this patch very well structured and executed in a very clean manner. It removes a lot of ambiguity, makes the overall design much easier to reason about and will definitely improve the overall health of the project. The benefits for LLVM Flang are almost immediate, see https://reviews.llvm.org/D157837. Thank you for implementing this, reviewing such patches is very satisfying and rewarding. This might cause some disruption to downstream consumers of this API and Options.td. Hopefully, "update_options_td_flags.py" that you've included should minimise that. I suggest "advertising" it in the summary a bit more. LGTM, great work, thank you! ================ Comment at: clang/lib/Driver/Driver.cpp:1941-1945 + // TODO: We're overriding the mask for flang here to keep this NFC for the + // option refactoring, but what we really need to do is annotate the flags + // that Flang uses. if (IsFlangMode()) + VisibilityMask = llvm::opt::Visibility(options::FlangOption); ---------------- Note to myself - I should update this in https://reviews.llvm.org/D157837. ================ Comment at: flang/docs/FlangDriver.md:248-250 +Options that are also supported by clang should explicitly specify `Default` in +`Vis`, and options that are only supported in Flang should not specify +`Default`. ---------------- For consistency ================ Comment at: llvm/unittests/Option/OptionParsingTest.cpp:18 +#pragma clang diagnostic ignored "-Wdeprecated-declarations" + ---------------- Why not just update the test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157151/new/ https://reviews.llvm.org/D157151 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
