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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: test/tools/obj2yaml/elf-symbol-visibility.yaml:14
+# CHECK-NEXT:   - Name: internal
+# CHECK-NEXT:     Other:
+# CHECK-NEXT:      - STV_INTERNAL
----------------
jhenderson wrote:
> grimar wrote:
> > 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.)
> I'm happy either way. I think it's a little confusing that obj2yaml might produce different output to what looks good and would be written in all the tests, so I think the extra complexity is probably worth it. It's not too bad, I think?
I also think the extra complexity is worth it.


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

https://reviews.llvm.org/D66886