Page MenuHomePhabricator

[clang][cli] Port DependencyOutput option flags to new option parsing system
ClosedPublic

Authored by jansvoboda11 on Jul 13 2020, 10:13 AM.

Diff Detail

Event Timeline

dang created this revision.Jul 13 2020, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2020, 10:13 AM
jansvoboda11 commandeered this revision.Nov 20 2020, 5:05 AM
jansvoboda11 added a reviewer: dang.
jansvoboda11 added a reviewer: dexonsmith.
jansvoboda11 added a subscriber: jansvoboda11.

Taking over this revision as Daniel is no longer involved.

Rebase, undo the move of options, introduce makeFlagToValueNormalizer

jansvoboda11 retitled this revision from Port DependencyOutput option flags to new option parsing system to [clang][cli] Port DependencyOutput option flags to new option parsing system.Nov 20 2020, 5:14 AM
jansvoboda11 edited the summary of this revision. (Show Details)
jansvoboda11 added inline comments.Nov 20 2020, 5:19 AM
clang/include/clang/Driver/Options.td
473

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.

dexonsmith requested changes to this revision.Nov 20 2020, 1:46 PM
dexonsmith added inline comments.
clang/include/clang/Driver/Options.td
473

This seems a lot cleaner, thanks!

clang/lib/Frontend/CompilerInvocation.cpp
139–145

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.

158–185

Please put this in an anonymous namespace.

161–162

Please drop the parameter names for TableIndex and Diags, since they aren't used.

169–172

Please declare this static.

This revision now requires changes to proceed.Nov 20 2020, 1:46 PM
dexonsmith added inline comments.Nov 20 2020, 3:37 PM
clang/lib/Frontend/CompilerInvocation.cpp
169–172

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.

Reduce template instantiations, remove names of unused parameters

Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 4:05 AM

Addressed review feedback.

jansvoboda11 edited the summary of this revision. (Show Details)Nov 27 2020, 8:29 AM
dexonsmith accepted this revision.Nov 30 2020, 1:09 PM

This LGTM, with one more nit to consider.

clang/lib/Frontend/CompilerInvocation.cpp
173

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.

This revision is now accepted and ready to land.Nov 30 2020, 1:09 PM

Remove unnecessary std namespace

This revision was landed with ongoing or failed builds.Dec 1 2020, 1:36 AM
This revision was automatically updated to reflect the committed changes.