Depends on D91861.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/include/clang/Driver/Options.td | ||
|---|---|---|
| 470 | The original patch used the following normalizer: 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. | |
| clang/include/clang/Driver/Options.td | ||
|---|---|---|
| 470 | This seems a lot cleaner, thanks! | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
| 141–147 | 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. | |
| 155–156 | Please put this in an anonymous namespace. | |
| 158–159 | Please drop the parameter names for TableIndex and Diags, since they aren't used. | |
| 166–169 | Please declare this static. | |
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 166–169 | BTW, I suspect we can save some compile time (once there are lots of these) with: template <
class T,
std::enable_if_t<
sizeof(T) <= sizeof(uint64_t) &&
std::is_trivially_convertible<T, uint64_t>::value &&
std::is_trivially_convertible<uint64_t, T>::value,
bool> = false>
FlagToValueNormalizer<uint64_t> makeFlagToValueNormalizer(T Value) {
return FlagToValueNormalizer<uint64_t>{std::move(Value)};
}
template <
class T,
std::enable_if_t<
!(sizeof(T) <= sizeof(uint64_t) &&
std::is_trivially_convertible<T, uint64_t>::value &&
std::is_trivially_convertible<uint64_t, T>::value),
bool> = false>
FlagToValueNormalizer<T> makeFlagToValueNormalizer(T Value) {
return FlagToValueNormalizer<T>{std::move(Value)};
}It'd be worth trying if/once the compile-time begins to climb to catch the enums (that aren't strongly typed) in a single instantiation. | |
Comment Actions
This LGTM, with one more nit to consider.
| clang/lib/Frontend/CompilerInvocation.cpp | ||
|---|---|---|
| 170 | I don't remember seeing std::uint*_t elsewhere in LLVM code; I think we tend to just say uint64_t (getting it from the C namespace). Unless you have a specific reason for adding it I suggest dropping the qualifier. | |
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.