This is an archive of the discontinued LLVM Phabricator instance.

[yaml] Add the ability to pass in context at the map level
ClosedPublic

Authored by zturner on Sep 1 2016, 3:06 PM.

Details

Summary

When doing mapRequired or mapOptional it is useful to be able to pass context to the operation. This has been partially addressed by having a void *context in the IO instance, but this is inflexible when you context is constructed on the fly, for example, and needs to be passed to only one specific mapping.

The solution implemented here adds an additional overload of mapRequired and mapOptional, taking a parameterized type. If you invoke this overload, you are expected to specialize a MappingTraits with not only the default implementation which takes a n IO& and a T &, but an additional overload that takes an IO&, T &, and Context &. This object is then passed into the map operation. The original no context overload is still required

Diff Detail

Event Timeline

zturner updated this revision to Diff 70079.Sep 1 2016, 3:06 PM
zturner retitled this revision from to [yaml] Add the ability to pass in context at the map level.
zturner updated this object.
zturner added reviewers: lhames, majnemer.
zturner added a subscriber: llvm-commits.
zturner updated this revision to Diff 70175.Sep 2 2016, 9:26 AM

It occurred to me that a limitation of my original patch is that it would have been impossible to define a mapping function that accepts a context if a specialization of MappingTraits had already been defined for T out of your control, since they all reused the same exact MappingTraits<T> specialization. To fix this mapping with context now requires an entirely different class. Instead of reusing MappingTraits<T>::mapping() it relies on MappingContextTraits<T, Context>::mapping(). Now one can have both a MappingContextTraits<T, Context> as well as a MappingTraits<T>, and if the non-context mapping functionality is identical, one could even define the context mapping traits like this:

template<>
struct MappingContextTraits<MyClass, MyContext> : public MappingTraits<MyClass> {
  static void mapping(IO &io, MyClass &Obj, MyContext &Context) {
    MappingTraits<MyClass>::mapping(io, Obj);
    // Do something with the context.
  }
};

Not sure why I added lhames, must have gotten confused. + people who are hopefully more appropriate.

Would someone mind taking a look at this? I've got 2-3 patches which can't go in until this one does.

Chandler,

AFAICT you are the only listed owner of Support. Would you mind reviewing this (or finding someone else to review it)?

chandlerc edited edge metadata.Sep 7 2016, 11:58 PM

I guess I can look. I was hoping someone with more familiarity with the code would, but unless Alex shows back up, that seems unlikely. And I understand you need to get unblocked.

I find the design pattern here pretty deeply confusing. But I don't think that it is at all related to your patch, but to my lack of familiarity with YAMLIO overall. Your patch does seem to fit the existing design patterns.

Since you might be able to, have you checked if MSVC 2013 is happy with the SFINAE trait you're using? I've had trouble getting those past it in the past.

Also a minor comment on the test below.

unittests/Support/YAMLIOTest.cpp
2305–2317

Shouldn't these be outside the llvm::yaml namespace and in the test's anonymous namespace?

Above, only the template specializations are in the llvm::yaml namespace which makes more sense to me.

I moved the structs out of the yaml namespace as you suggested. Tested on VS 2013 and everything works fine. I'll upload a new patch if you want, but the only change was simply moving the classes in the unit tests to the global namespace.

And yes, I agree with you it uses some confusing design patterns. I can try to explain it in more detail if you like, let me know if that would be helpful.

chandlerc accepted this revision.Sep 8 2016, 10:37 AM
chandlerc edited edge metadata.

LGTM with the previous changes and another change below.

include/llvm/Support/YAMLTraits.h
726

Is this in some 'internal' or 'detail' namespace that I don't see? If not, I think it probably should be. Seems like too generic of a name.

Also, probably should be doMapping to be consistent with naming conventions (not that this file is terribly consistent with the rest of LLVM's conventions).

This revision is now accepted and ready to land.Sep 8 2016, 10:37 AM
This revision was automatically updated to reflect the committed changes.