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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
