[PATCH] D77844: [ObjectYAML][MachO] Add support for relocations

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

The change about ObjectYAML/obj2yaml looks generally fine, so LGTM. 
(Perhaps would be nice to see an approve from someone who knows MachO better too)

I've put the last nit inline. It is probably up to you to decide keep it or to change,
as I have no horse in this race, because MachO is not my area.



================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:339
+  const unsigned IsExtern = R.is_extern;
+  const unsigned Type = R.type;
+  if (IsObjLittleEndian)
----------------
alexshap wrote:
> grimar wrote:
> > alexshap wrote:
> > > grimar wrote:
> > > > I think you can inline these 5 `const unsigned` variables.
> > > so basically here integer promotion rules come into play, e.g. if i am not mistaken bool would be implicitly casted to int (signed) and then left shifted, since we are shifting to the left everything would be okay (even though int is signed https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators), but I somehow wanted to avoid these hidden implicit transformations and make things explicit.  
> > I do not mind leaving `bool`s alone (I've not noticed them honestly),
> > but why not to inline other variables at least to make the code shorter?
> > 
> > ```
> >   const unsigned IsPCRel = R.is_pcrel ? 1 : 0;
> >   const unsigned IsExtern = R.is_extern ? 1 : 0;
> >   MRE.r_word1 = ((unsigned)R.symbolnum << 0) | (IsPCRel << 24) |
> >                 ((unsigned)R.length << 25) | (IsExtern << 27) |
> >                 ((unsigned)R.type << 28);
> > ```
> not sure it provides any improvements in terms of readability, 
> additionally C-style casts might disappoint clang-tidy. I find it easier to read the current version.
> not sure it provides any improvements in terms of readability,

The following variables looks excessive as they do not introduce new meaning.
Removing them would help in that sense.
```
 const unsigned IsPCRel = R.is_pcrel;
 const unsigned IsExtern = R.is_extern;
 const unsigned Type = R.type;
```

> additionally C-style casts might disappoint clang-tidy.

We have many places in LLVM where `(unsigned)-1`, `(uint64_t)-1` and other similar conversions are used,
so I'd say it is very common.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77844