[PATCH] D66886: [yaml2obj][obj2yaml] - Use a single "Other" field instead of "Other", "Visibility" and "StOther".

[George Rimar via Phabricator via llvm-commits]

grimar marked an inline comment as done.
grimar added inline comments.


================
Comment at: test/tools/obj2yaml/elf-symbol-visibility.yaml:14
+# CHECK-NEXT:   - Name: internal
+# CHECK-NEXT:     Other:
+# CHECK-NEXT:      - STV_INTERNAL
----------------
MaskRay wrote:
> MaskRay wrote:
> > jhenderson wrote:
> > > Is there any way straightforward way of forcing obj2yaml to produce output in the form `Other: [ STV_HIDDEN ]` etc?
> > Not straightforward but I managed to find an approach..
> > 
> > ```
> >  struct NormalizedOther {
> > +  struct Piece {
> > +    std::string Str;
> > +  };
> >  ...
> > -  Optional<std::vector<StringRef>> Other;
> > +  Optional<std::vector<Piece>> Other;
> >    std::string UnknownFlagsHolder;
> >  };
> >  } // namespace
> >  
> > +template<>
> > +struct ScalarTraits<NormalizedOther::Piece> {
> > +  static void output(const NormalizedOther::Piece &Val, void *, raw_ostream &Out) {
> > +    Out << Val.Str;
> > +  }
> > +  static StringRef input(StringRef Scalar, void *, NormalizedOther::Piece &Val) {
> > +    Val.Str = Scalar.str();
> > +    return {};
> > +  }
> > +  static QuotingType mustQuote(StringRef) { return QuotingType::None; }
> > +};
> > +template<> struct SequenceElementTraits<NormalizedOther::Piece> {
> > +  static const bool flow = true;
> > +};
> > ```
> > 
> > This way obj2yaml will dump: `Other: [ STV_HIDDEN ]`
> Oh, I think StringRef should be fine.
> 
> ```
> +  struct Piece {
> +    StringRef Str;
> +  };
> ```
Thanks a lot for this! I was wondering yesterday how to achieve it, but did not have chance to experiment with it.

My question is: do we want it? I.e it adds a bit of complexity to the code, but makes the output better.
I really like it the `Other: [ STV_HIDDEN ]` output style more, just want to check this point before addressing.

(I'll try to address this and all other comments today.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66886/new/

https://reviews.llvm.org/D66886