awarzynski added a comment. Makes sense, thanks for working on this! Some minor comments below and inline.
From your summary: > This is done by using clang tablegen What do you mean by "clang tablegen"? Is it Clang's clang_tablegen <https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/clang/cmake/modules/AddClang.cmake#L4-L32> CMake function? > from options.td Options.td ;-) > shared with clang Most people associate "clang" with the executable. You may want to use "Clang" to avoid confusion (IIUC, you are referring to the sub-project rather than the executable here). > specific flang flags Flang :) (same rationale as above) ================ Comment at: clang/utils/TableGen/ClangOptionDocEmitter.cpp:329 raw_ostream &OS) { - if (isExcluded(Option.Option, DocInfo)) + if (isExcluded(Option.Option, DocInfo) || !isIncluded(Option.Option, DocInfo)) return; ---------------- I think that the way this is written at the moment is a bit counterintuitive (i.e. "if excluded or not included"). This is really down to how these include/exclude flags are used in various other places :( (i.e. there's little that we can do about it). I think that it would make more sense to follow the style from [[ https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/llvm/lib/Option/OptTable.cpp#L656-L659 | OptTable.cpp ]]. In particular (pseudo code): ``` if (excluded) return; if (included_not_empty && !included) return; ``` This way it becomes clear that `IncludedFlags` (from "FlangOptionsDocs.td") is only taken into account when not empty. I would also add an assert in `isIncluded`: ``` assert(DocInfo->getValue("IncludedFlags") && Missing includeFlags); ``` WDYT? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129864/new/ https://reviews.llvm.org/D129864 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
