[PATCH] D58103: [llvm-readobj] Only allow 4-byte pr_data and refactor GNU_PROPERTY_X86_FEATURE_1_AND processing a bit

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D58103#1394387 <https://reviews.llvm.org/D58103#1394387>, @MaskRay wrote:

> In D58103#1394386 <https://reviews.llvm.org/D58103#1394386>, @grimar wrote:
>
> > Looks correct, FTR ABI link is https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf.
> >
> > Regarding the refactoring, in my opinion, the original code was simpler. I would leave it as is.
> >  (at least refactoring change could be in a separate patch, if you want to do it)
>
>
> For `GNU_PROPERTY_X86_FEATURE_1_AND`, there are only 2 bits. When people start implementing 
>  `GNU_PROPERTY_X86_ISA_1_USED` `GNU_PROPERTY_X86_ISA_2_USED` `GNU_PROPERTY_X86_COMPAT_ISA_1_USED`, this style will become better :)


There are different ways to do a refactoring here. For example, it could probably be:

>   auto DumpFlag = [](StringRef Name, uint64_t Flag, uint32_t &Val) {
>     if (Val & Flag) {
>       Val &= ~Flag;
>       OS << Name;
>       if (Val)
>         OS << ", ";
>     }
>   };
>    
>   DumpFlag("IBT", GNU_PROPERTY_X86_FEATURE_1_IBT, pr_data);
>   DumpFlag("SHSTK", GNU_PROPERTY_X86_FEATURE_1_SHSTK, pr_data);

Which still looks simpler to me, but you or other people might disagree,
so probably it is better to discuss the refactoring in a different review.

My main point is that functional and refactoring changes
should not be mixed in the same patch.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58103