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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:339
+  const unsigned IsExtern = R.is_extern;
+  const unsigned Type = R.type;
+  if (IsObjLittleEndian)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77844