Extend llvm::yaml::SequenceTraits for batch insertion

[aleclearmind]

We need this change in order to do this.
Comments?

[drblallo]

if i understand correctly the issue is that there are some custom classes that do not implements the serializable traits but are serializable in some other way and the removed overloads prevents selecting the correct one?

[aleclearmind]

if i understand correctly the issue is that there are some custom classes that do not implements the serializable traits but are serializable in some other way and the removed overloads prevents selecting the correct one?

The problem is that LLVM has an implementation of yamlize that is enable_ifd if all the other known implementation do not fit.
The point is that the enable_if condition does not consider implementation that other users might want to introduce (such as ours).

Long story short, the fallback implementation (which is used to provide "useful" error information to the developer in case he's using a class for which GraphTraits are not implemented) prevents using our custom yamlize function.

Dropping the fallback implementation solves the problem, but we no longer have nice errors if we're trying to use YAMLTraits with a class for which they are actually not implemented.

Makes sense?

[drblallo]

then wouldn't be better to add

enabled_if<missing_traits and not is_KeyedObjectContainer_t<T>>

?

since we must operate changes on llvm we may as well improve it rather than just removing a feature.
I guess that would require to move is_keyedObject to llvm?

[aleclearmind]

I guess that would require to move is_keyedObject to llvm?

Yeah, we don't want to do that...

[pfez]

In LLVM's code (llvm/include/llvm/Support/YAMLTraits.h:720 I see the following,

  template <typename T, typename Context>
  struct missingTraits
      : public std::integral_constant<bool,
                                      !has_ScalarEnumerationTraits<T>::value &&
                                          !has_ScalarBitSetTraits<T>::value &&
                                          !has_ScalarTraits<T>::value &&
                                          !has_BlockScalarTraits<T>::value &&
                                          !has_TaggedScalarTraits<T>::value &&
                                          !has_MappingTraits<T, Context>::value &&
                                          !has_SequenceTraits<T>::value &&
                                          !has_CustomMappingTraits<T>::value &&
                                          !has_DocumentListTraits<T>::value &&
                                          !has_PolymorphicTraits<T>::value> {};

Can't we massage our own data structures that need to be serialized so that they make has_MappingTraits or has_CustomMappingTraits or any of the other relevant cases true?

I'm not that much into the Model data structures yet, but it looks like we should be able to make them fit into one of the foreseen cases. Is that not the case?

[aleclearmind]

Can't we massage our own data structures that need to be serialized so that they make has_MappingTraits or has_CustomMappingTraits or any of the other relevant cases true?

Nope, those you mentioned create YAML "mappings" (a "map"), in this situation we want a YAML "sequence" (a "vector").

We might create a new trait in LLVM, CustomSequenceTraits, and move the bulk of our logic in a yamlize specialization using that trait. Then, in revng, we can specialize that trait for our data structures.

This is probably the cleanest way to go. Users are not supposed to specialize yamlize after all.

Problem is: it might take a bit of time and this issue is annoying @drblallo and Alain.
I'll see if it's doable.

[pfez]

Yes! The best thing would be if we manage to customize it using has_SequenceTraits. But yeah, try to understand how long it may take. If it's blocking @drblallo we can go ahead with how it is right now.

If we go ahead like it is right now, please add a comment for future reference on the right way to do it. Or at least mention it in the commit message.

[aleclearmind]

Yes! The best thing would be if we manage to customize it using has_SequenceTraits.

Wait, we need CustomSequenceTraits. Regular SequenceTraits assume you have a pre-.resized vector(-like), which is not the situation we're in.
Nevertheless, it can be done with a custom trait, or maybe changing the existign one in a backward compatible way.

[pfez]

Yes! The best thing would be if we manage to customize it using has_SequenceTraits.

Wait, we need CustomSequenceTraits. Regular SequenceTraits assume you have a pre-.resized vector(-like), which is not the situation we're in.

Yeah, sorry, my bad. CustomSequenceTraits. Sloppy copy-pasting.

[aleclearmind]

New usage:

template<typename T>
struct llvm::yaml::SequenceTraits<T, enable_if_is_KeyedObjectContainer_t<T>> {
  static size_t size(IO &io, T &Seq) { return Seq.size(); }

  class Inserter {
  private:
    using value_type = typename T::value_type;
    using KOT = KeyedObjectTraits<value_type>;
    using key_type = decltype(KOT::key(std::declval<value_type>()));

  private:
    typename T::BatchInserter BatchInserter;
    value_type Instance;

  public:
    Inserter(IO &io, T &Seq) :
      BatchInserter(Seq.batch_insert()), Instance(KOT::fromKey(key_type())) {}

    auto &preflightElement(unsigned) { return Instance; }

    void postflightElement(unsigned) {
      BatchInserter.insert(Instance);
      Instance = KOT::fromKey(key_type());
    };
  };
};

[aleclearmind]

Merged in befde3c.
Thanks everyone for the feedback.