This is an archive of the discontinued LLVM Phabricator instance.

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

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

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.

This seems a lot cleaner, thanks!


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*/...) {

Note that it'd also be nice to make this static and drop unused parameter names, as I've done here.


Please put this in an anonymous namespace.


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


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

BTW, I suspect we can save some compile time (once there are lots of these) with:

template <
  class 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,
      !(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.


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.