[Lldb-commits] [PATCH] D149774: [lldb] Use templates to simplify {Get, Set}PropertyAtIndex (NFC)

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 2 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Interpreter/OptionValue.h:325-344
+  template <typename T,
+            std::enable_if_t<!std::is_pointer<T>::value, bool> = true>
+  std::optional<T> GetValueAs() const {
+    if constexpr (std::is_same_v<T, uint64_t>)
+      return GetUInt64Value();
+    if constexpr (std::is_same_v<T, int64_t>)
+      return GetSInt64Value();
----------------
mib wrote:
> nit: In the template argument, you use `std::is_pointer<T>::value` instead of `std::is_pointer_v<T>` and the in the if statement you do the opposite (`std::is_same_v<T>` vs `std::is_same<T>::value`). I personally not a fan of the `_v` alias but what I'm saying here is it would be good to stay consistent.
Good point, I've unified everything to use the `_v` variant. 


================
Comment at: lldb/source/Interpreter/Property.cpp:229
   }
+  assert(m_value_sp && "invalid property definition");
 }
----------------
mib wrote:
> May be we should print a warning or error to the user ?
Properties are all tablegen'd so if this fires we did something wrong. There's nothing actionable here for a user. 


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

https://reviews.llvm.org/D149774