[PATCH] D66886: [yaml2obj][obj2yaml] - Use a single "Other" field instead of "Other", "Visibility" and "StOther".
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 29 09:51:18 PDT 2019
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
More information about the llvm-commits
mailing list