[Lldb-commits] [PATCH] D16627: Add support to detect arm hard float ABI based binaries for ABISysV_arm

[Tamas Berghammer via lldb-commits]

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

Looks good with a few nits inline


================
Comment at: source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:418
@@ +417,3 @@
+bool
+ABISysV_arm::IsArmHardFloat (Thread *thread) const
+{
----------------
(nit): I would suggest to pass in the thread by reference because the function have no meaning with a NULL thread

================
Comment at: source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:427
@@ +426,3 @@
+            const ArchSpec &arch (process_sp->GetTarget().GetArchitecture());
+            if (arch.GetFlags() == ArchSpec::eARM_abi_hard_float)
+            {
----------------
tberghammer wrote:
> (nit): You can write it much simpler like this:
> 
> ```
> return (arch.GetFlags() & ArchSpec::eARM_abi_hard_float) != 0;
> ```
This will break if we add a new (independent) architecture flag for arm in the future. Please only check if the given bit is set instead of comparing for equality.

================
Comment at: source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:427-430
@@ +426,6 @@
+            const ArchSpec &arch (process_sp->GetTarget().GetArchitecture());
+            if (arch.GetFlags() == ArchSpec::eARM_abi_hard_float)
+            {
+                is_armhf = true;
+            }
+        }
----------------
(nit): You can write it much simpler like this:

```
return (arch.GetFlags() & ArchSpec::eARM_abi_hard_float) != 0;
```

================
Comment at: source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:539
@@ +538,3 @@
+
+                    if (!IsArmHardFloat(&thread))
+                    {
----------------
(nit); Please revert the order of the if and the else case so you don't have to negate the condition


http://reviews.llvm.org/D16627