zturner added inline comments.
================
Comment at: include/lldb/Host/HostInfoBase.h:85
+ //---------------------------------------------------------------------------
+ /// If the triple contains not specify the vendor, os, and environment parts,
+ /// we "augment" these using information from the host and return the
----------------
s/contains/does/
================
Comment at: include/lldb/Target/Platform.h:524
+ //---------------------------------------------------------------------------
+ ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
+
----------------
Should this function be marked `const`?
Another possible interface would be `void AugmentArchSpec(ArchSpec &spec);`
================
Comment at: source/API/SBDebugger.cpp:698
+ ArchSpec arch = Platform::GetAugmentedArchSpec(
+ m_opaque_sp->GetPlatformList().GetSelectedPlatform().get(), arch_name);
TargetSP target_sp(
----------------
Is `arch_name` here actually a triple? If so, is it worth renaming this
variable?
================
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));
}
----------------
Why does this one use `HostInfo` instead of `Platform`?
================
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));
----------------
Should this function be marked `const`?
================
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());
----------------
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.
================
Comment at: source/Target/Platform.cpp:975-976
+ return ArchSpec();
+ if (ArchSpec::ContainsMoreThanArch(triple))
+ return ArchSpec(triple);
+
----------------
Same here, this method ends up normalizing twice, once inside of
`ContainsMoreThanArch` and once inside of this function.
================
Comment at: source/Target/Platform.cpp:984
+ llvm::Triple normalized_triple(llvm::Triple::normalize(triple));
+ if (compatible_arch.IsValid()) {
+ const llvm::Triple &compatible_triple = compatible_arch.GetTriple();
----------------
Can you early-return here?
================
Comment at: source/Target/Platform.cpp:986-991
+ if (normalized_triple.getVendorName().empty())
+ normalized_triple.setVendor(compatible_triple.getVendor());
+ if (normalized_triple.getOSName().empty())
+ normalized_triple.setOS(compatible_triple.getOS());
+ if (normalized_triple.getEnvironmentName().empty())
+ normalized_triple.setEnvironment(compatible_triple.getEnvironment());
----------------
Are these cases even possible? Why would the vendor and os ever be empty? I
thought only the environment could be empty.
https://reviews.llvm.org/D39387
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
