[PATCH] D36823: [LLD][ELF] Read ARM BuildAttributes section to determine supported features.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few non-strong suggestions from me.



================
Comment at: ELF/Config.h:112-114
+  bool ARMHasBlx = false;
+  bool ARMHasMovtMovw = false;
+  bool ARMJ1J2BranchEncoding = false;
----------------
peter.smith wrote:
> emaste wrote:
> > Should these now be `ArmHas...` etc.? (i.e., new capitalization as mentioned on llvm-weekly)
> I haven't got a good answer for you unfortunately. The Arm form is correct according to the new capitalization, but ARM is more consistent with the names throughout the rest of the code. My guess was that consistency would be preferred although I'm very happy to change to use Arm.  
So you use this 3 only for producing a warning currently. And them are non a command line options,
but target specific flags, so I wonder will it be better to make them be a part of ARM target class ?
Or/and may be group them in a struct like `ARMFeatures` ?


================
Comment at: ELF/InputFiles.cpp:358
+// permitted to use them.
+static void updateSupportedARMFeatures(const ARMAttributeParser &Attributes) {
+  if (Attributes.hasAttribute(ARMBuildAttrs::CPU_arch)) {
----------------
Should this be in ARM.cpp ? Probably a method of ARM target (non-virtual) ?


================
Comment at: ELF/InputFiles.cpp:372
+    case ARMBuildAttrs::v6KZ:
+    case ARMBuildAttrs::v6K:
+      Config->ARMHasBlx = true;
----------------
Is `v6T2` intentionally missing from this list ?


https://reviews.llvm.org/D36823