awarzynski added a comment. What's the overall design goal here? 100% consistency with Clang? Could this be documented?
================ Comment at: clang/include/clang/Driver/ToolChain.h:23 #include "llvm/ADT/StringRef.h" -#include "llvm/ADT/Triple.h" #include "llvm/Frontend/Debug/Options.h" ---------------- Unrelated change? ================ Comment at: clang/lib/Driver/ToolChains/Clang.h:16 #include "clang/Driver/Types.h" -#include "llvm/ADT/Triple.h" #include "llvm/Frontend/Debug/Options.h" ---------------- Unrelated change? ================ Comment at: clang/lib/Driver/ToolChains/Flang.cpp:33 +// to the corresponding DebugInfoKind. +static llvm::codegenoptions::DebugInfoKind DebugLevelToInfoKind(const Arg &A) { + assert(A.getOption().matches(options::OPT_gN_Group) && ---------------- Looks identical to https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/lib/Driver/ToolChains/Clang.cpp#L406-L420. Why not move to https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp? ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:127 + unsigned val = + llvm::StringSwitch<unsigned>(arg->getValue()) + .Case("line-tables-only", llvm::codegenoptions::DebugLineTablesOnly) ---------------- 1. Why `unsigned` instead of [[ https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/include/clang/Basic/DebugInfoOptions.h#L20-L55 | DebugInfoKind ]] 2. Why not use `std::optional`, e.g. `llvm::StringSwitch<std::optional<DebugInfoKind>>arg->getValue())`? This way you could avoid magic numbers like `~0U`. ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:137 + if (val == ~0U) { + diags.Report(clang::diag::err_drv_invalid_value) + << arg->getAsString(args) << arg->getValue(); ---------------- Please test this diagnostic. Also, if this is an error then you should return immediately and signal that this method has failed (e.g. `return false;`). ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:144 + val != llvm::codegenoptions::NoDebugInfo) { + // TODO: This is not a great warning message, could be improved + const auto debugErr = diags.getCustomDiagID( ---------------- Please improve it :) In particular, you are testing for `DebugLineTablesOnly` and `NoDebugInfo`, yet the diagnostic refers to `-g1`. ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:145 + // TODO: This is not a great warning message, could be improved + const auto debugErr = diags.getCustomDiagID( + clang::DiagnosticsEngine::Warning, ---------------- Looks like you are creating a warning rather than an error: `debugErr` --> `debugWarn` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146814/new/ https://reviews.llvm.org/D146814 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits