zturner added inline comments.
================
Comment at: lit/Modules/Breakpad/lit.local.cfg:1
+config.suffixes = ['.test']
----------------
This shouldn't be necessary, the top-level `lit.cfg.py` already recognizes
`.test` extension. You only need a lit.local.cfg if you're changing something.
================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:20-43
+static llvm::Triple::OSType toOS(llvm::StringRef str) {
+ using llvm::Triple;
+ return llvm::StringSwitch<Triple::OSType>(str)
+ .Case("Linux", Triple::Linux)
+ .Case("mac", Triple::MacOSX)
+ .Case("windows", Triple::Win32)
+ .Default(Triple::UnknownOS);
----------------
LLVM already has these functions in `Triple.cpp`, but they are hidden as
private implementations in the CPP file. Perhaps we should expose them from
headers in Triple.h.
================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:56-59
+ if (to_integer(str.substr(0, 8), t, 16))
+ data.uuid1 = t;
+ else
+ return UUID();
----------------
Consider using `StringRef::consumeInteger()` here.
================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:60-77
+ for (int i = 0; i < 2; ++i) {
+ if (to_integer(str.substr(8 + i * 4, 4), t, 16))
+ data.uuid2[i] = t;
+ else
+ return UUID();
+ }
+ for (int i = 0; i < 8; ++i) {
----------------
Similarly for these lines, by using `consume` functions everywhere we can get
rid of a lot of the math and I think make the code easier to follow.
================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:92-101
+ std::tie(token, line) = getToken(line);
+ llvm::Triple triple;
+ triple.setOS(toOS(token));
+ if (triple.getOS() == llvm::Triple::UnknownOS)
+ return llvm::None;
+
+ std::tie(token, line) = getToken(line);
----------------
Instead of having the custom parsing functions above, how about just:
```
std::tie(os, line) = getToken(line);
std::tie(arch, line) = getToken(line);
llvm::Triple triple(os, "unknown", arch);
if (triple.getArch() == Unknown || triple.getOS() == Unknown)
return llvm::None;
```
This way we don't even need to expose the parse functions I commented on
earlier, and we can just delete them.
================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:160-161
+ }
+ llvm::StringRef text(reinterpret_cast<const char *>(data_sp->GetBytes()),
+ data_sp->GetByteSize());
+
----------------
We have `GetData()` which returns an `ArrayRef`, and another function
`toStringRef` which converts an `ArrayRef` to a `StringRef`. So this might be
cleaner to write as `auto text = llvm::toStringRef(data_sp->GetData());`
================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:74
+
+ bool IsStripped() override { return false; }
+
----------------
Is this always true for breakpad files?
================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:98
+
+private:
+ struct Header {
----------------
lemo wrote:
> Nit: I personally prefer not to mix data, type and function members in the
> same "access" section - is there an LLVM/LLDB guideline which requires
> everything in the same place?
>
> If not, can you please add a private section for the destructor, followed by
> a section for the private data members?
Given that we don't actually store an instance of the header anywhere, we just
use it as a constructor parameter, perhaps we could go one step further and
move this entire type to an anonymous namespace in the cpp file, and update the
constructor to take an `ArchSpec` and a `UUID`.
I prefer to avoid nested classes wherever possible since it clutters up the
interface, so hiding it to the cpp file is nice.
================
Comment at: tools/lldb-test/SystemInitializerTest.cpp:120
+ breakpad::ObjectFileBreakpad::Initialize();
ObjectFileELF::Initialize();
----------------
Shouldn't we also initialize this in `SystemInitializerFull`?
================
Comment at: tools/lldb-test/lldb-test.cpp:737
+ auto *ObjectPtr = ModulePtr->GetObjectFile();
+
----------------
I would use an explicit type spelling here, but since the function is called
`GetObjectFile`, I don't feel too strongly. It's pretty clear what the return
type is.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55214/new/
https://reviews.llvm.org/D55214
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
