[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: lib/ObjectYAML/ELFYAML.cpp:877-880
+    llvm::WithColor::error()
+        << "an unknown value is used for symbol's 'Other' field: " << Name
+        << ".\n";
+    exit(1);
----------------
grimar wrote:
> MaskRay wrote:
> > jhenderson wrote:
> > > Do other yaml2obj/obj2yaml error messages end with a full stop?
> > > 
> > > Also, this error reporting method here makes me uncomfortable, since we're in the middle of a library. Is there any sensible way to propagate the error back up the stack?
> > In ELFYAML.h
> > 
> > ```
> >   template <>
> >   struct MappingTraits<ELFYAML::Symbol> {
> >   ...
> > +   std::string Err;
> > ```
> > 
> > In ELFYAML.cpp,
> > 
> > ```
> >   StringRef MappingTraits<ELFYAML::Symbol>::validate(IO &IO,
> >                                                    ELFYAML::Symbol &Symbol) {
> > +  if (!Err.empty())
> > +    return Err;
> > ...
> >   }
> > 
> > -   NormalizedOther(IO &IO, Optional<uint8_t> Original) : YamlIO(IO) {
> > +  NormalizedOther(IO &IO, ELFYAML::Symbol &Sym) : YamlIO(IO) {
> > 
> > -  MappingNormalization<NormalizedOther, Optional<uint8_t>> Keys(IO, Symbol.Other);
> > +  MappingNormalization<NormalizedOther, ELFYAML::Symbol> Keys(IO, Symbol);
> > ```
> > 
> > I haven't verified if this approach works, if it does, the error can be propagated back via `validate()`.
> > Do other yaml2obj/obj2yaml error messages end with a full stop?
> 
> Yes.
> 
> > Also, this error reporting method here makes me uncomfortable, since we're in the middle of a library. Is there any sensible way to propagate the error back up the stack?
> 
> This is a common problem, `exit(1)` is already used in a 3 more places in lib\ObjectYAML. Having that, what about if I suggest a follow-up refactoring(s) for all those places and this one?
> This is a common problem, exit(1) is already used in a 3 more places in lib\ObjectYAML. 

Yes. I think obj2yaml functions have better error handling than yaml2obj function. It should be fine to fix the error propagating problem later.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:962
+  // represents st_other and holds the symbol visibility. However, on some
+  // platforms, it can contain bit field and regular values, or even sometimes a
+  // crazy mix of them (see comments for NormalizedOther). Because of this, we
----------------
bit field -> bit fields


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

https://reviews.llvm.org/D66886