[PATCH] D83694: [clang][cli] Port DependencyOutput option flags to new option parsing system
Duncan P. N. Exon Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 20 13:46:13 PST 2020
dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang/include/clang/Driver/Options.td:473
+ MarshallingInfoFlag<"DependencyOutputOpts.OutputFormat", "DependencyOutputFormat::Make">,
+ Normalizer<"makeFlagToValueNormalizer(DependencyOutputFormat::NMake)">;
def Mach : Flag<["-"], "Mach">, Group<Link_Group>;
----------------
jansvoboda11 wrote:
> The original patch used the following normalizer:
> `(normalizeFlagToValue<DependencyOutputFormat, DependencyOutputFormat::NMake>)`.
>
> I wanted to remove the parenthesis and redundant type argument, hence `makeFlagToValueNormalizer`. This could be useful later on when normalizing flags to non-literal types.
This seems a lot cleaner, thanks!
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:137-143
+template <typename T>
void denormalizeSimpleFlag(SmallVectorImpl<const char *> &Args,
const char *Spelling,
CompilerInvocation::StringAllocator SA,
- unsigned TableIndex, unsigned Value) {
+ unsigned TableIndex, T Value) {
Args.push_back(Spelling);
}
----------------
On the switch from `T` to `unsigned`, it'd be nice to avoid a separate instantiation for each enumeration, since the code is identical... it'll just bloat compile time for no reason. Maybe:
```
/// The tblgen-erated code passes in a fifth parameter of an arbitrary type, but
/// denormalizeSimpleFlags never looks at it. Avoid bloating compile-time with
/// unnecessary template instantiations and just ignore it with a variadic
/// argument.
static void denormalizeSimpleFlag(
SmallVectorImpl<const char *> &Args, const char *Spelling,
CompilerInvocation::StringAllocator, unsigned, /*T*/...) {
Args.push_back(Spelling);
}
```
Note that it'd also be nice to make this `static` and drop unused parameter names, as I've done here.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:145-154
+template <typename T> struct FlagToValueNormalizer {
+ T Value;
+
+ llvm::Optional<T> operator()(OptSpecifier Opt, unsigned TableIndex,
+ const ArgList &Args, DiagnosticsEngine &Diags) {
+ if (Args.hasArg(Opt))
+ return Value;
----------------
Please put this in an anonymous namespace.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:148-149
+
+ llvm::Optional<T> operator()(OptSpecifier Opt, unsigned TableIndex,
+ const ArgList &Args, DiagnosticsEngine &Diags) {
+ if (Args.hasArg(Opt))
----------------
Please drop the parameter names for `TableIndex` and `Diags`, since they aren't used.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:156-159
+template <typename T>
+FlagToValueNormalizer<T> makeFlagToValueNormalizer(T Value) {
+ return FlagToValueNormalizer<T>{std::move(Value)};
}
----------------
Please declare this `static`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83694/new/
https://reviews.llvm.org/D83694
More information about the cfe-commits
mailing list