[Lldb-commits] [PATCH] D39387: Invert ArchSpec<->Platform dependency

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.


================
Comment at: include/lldb/Target/Platform.h:524
+  //---------------------------------------------------------------------------
+  ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
+
----------------
zturner wrote:
> Should this function be marked `const`?
> 
> Another possible interface would be `void AugmentArchSpec(ArchSpec &spec);`  
Unfortunately it's not possible, as it ends up calling a couple of other function, which are not const, and some of them are virtual.

 I don't think the other signature would be very practical, as all the present uses pass in a string, and they don't have an ArchSpec around.


================
Comment at: source/API/SBDebugger.cpp:698
+    ArchSpec arch = Platform::GetAugmentedArchSpec(
+        m_opaque_sp->GetPlatformList().GetSelectedPlatform().get(), arch_name);
     TargetSP target_sp(
----------------
zturner wrote:
> Is `arch_name` here actually a triple?  If so, is it worth renaming this variable?
As discussed, this is probably expected to only be an architecture name (although, nothing is stopping it from being a triple).


================
Comment at: source/API/SBInstruction.cpp:263
   if (inst_sp && triple) {
-    lldb_private::ArchSpec arch(triple, NULL);
-    return inst_sp->DumpEmulation(arch);
+    return inst_sp->DumpEmulation(HostInfo::GetAugmentedArchSpec(triple));
   }
----------------
zturner wrote:
> Why does this one use `HostInfo` instead of `Platform`?
Since it is hard-coding a null platform, I figured it is best to be explicit that it is going to pick up data from the host.

As to why is it doing that, I have no idea, but there doesn't seem to be an easy way to fetch a platform instance here.


================
Comment at: source/Core/ArchSpec.cpp:889
 
-bool ArchSpec::SetTriple(llvm::StringRef triple, Platform *platform) {
-  if (triple.empty()) {
-    Clear();
-    return false;
-  }
-  if (ParseMachCPUDashSubtypeTriple(triple, *this))
-    return true;
-
-  if (triple.startswith(LLDB_ARCH_DEFAULT)) {
-    // Special case for the current host default architectures...
-    if (triple.equals(LLDB_ARCH_DEFAULT_32BIT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKind32);
-    else if (triple.equals(LLDB_ARCH_DEFAULT_64BIT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKind64);
-    else if (triple.equals(LLDB_ARCH_DEFAULT))
-      *this = HostInfo::GetArchitecture(HostInfo::eArchKindDefault);
-    return IsValid();
-  }
-
-  ArchSpec raw_arch(triple);
-
+bool ArchSpec::ContainsMoreThanArch(llvm::StringRef triple) {
   llvm::Triple normalized_triple(llvm::Triple::normalize(triple));
----------------
zturner wrote:
> Should this function be marked `const`?
This function is static.


================
Comment at: source/Host/common/HostInfoBase.cpp:257-261
+  if (ArchSpec::ContainsMoreThanArch(triple))
+    return ArchSpec(triple);
+
+  llvm::Triple normalized_triple(llvm::Triple::normalize(triple));
+  llvm::Triple host_triple(llvm::sys::getDefaultTargetTriple());
----------------
zturner wrote:
> I don't *think* this is very performance sensitive, but it's unfortunate that this ends up calling `normalize` twice.  If there's a clean way to re-write it that would probably be nice.
I've made the function assume that the incoming triple in normalized. That way, we can just normalize once here, and then pass the normalized triple. I've also inverted the function name to `ContainsOnlyArch` which is slightly clearer, I think.


https://reviews.llvm.org/D39387