- User Since
- Jan 31 2016, 7:15 AM (373 w, 2 d)
Oops, meant to request changes rather than accept this.
LGTM. I think it would be worth adding that information to the top of the file to prevent similar mistakes in the future.
Sun, Mar 26
Sat, Mar 25
Limit test to Darwin as it relies on lipo
Fri, Mar 24
Thu, Mar 23
Wed, Mar 22
Tue, Mar 21
Mon, Mar 20
Can we do something similar for plugins?
LGTM modulo comments
We're only using the OS part of the triple/ArchSpec so there should be a 1:1 mapping with the platform. I don't see any issue with this. LGTM.
Fri, Mar 17
Thanks, this is great. LGTM.
Thu, Mar 16
LGTM. I probably would've gone with expo but as expression is the canonical name of the command, this makes sense.
Wed, Mar 15
Thanks for adding this, Jim. This is one of those things that comes up one in a while when it confuses our users. It's great we'll be able to help them adopt this.
Tue, Mar 14
LGTM modulo some nits in the comments.
LGMT with some wordsmithing
LGTM with Pavel's comments.
Mon, Mar 13
LGTM modulo the else-after-return.
LGTM if @clayborg is happy
Thu, Mar 9
Wed, Mar 8
Some code style nits but the change itself LGTM.
- Remove 9d311dd6a71b from patch
- Use EXPECT_THAT_EXPECTED
- Add unit tests for JSON deserialization
- Add unit tests for converting JSONSymbol to Symbol
Tue, Mar 7
- Remove section field
- Support raw value symbols
- Update test
Mon, Mar 6
Implement Greg's features. I'll add a unit test for the deserialization later today.
Fri, Mar 3
Address Greg's feedback
LGTM modulo inline comments.
LGTM. Can we turn that crash into a small test case?
Thanks for the feedback Greg, they're all great suggestions.
Thu, Mar 2
This will break the Xcode build again. It's being discussed here: https://github.com/llvm/llvm-project/issues/60314
Tue, Feb 28
LGTM with two small nits.
The implementation looks good. I left some inline comments with suggestions for names that more closely match that of the STL containers to make the class feel more familiar to those who are used to them.
You're passing is_callback by value so the const is close to redundant. I think there's an "Effective C++" chapter dedicated to this. LLVM is pretty conservative in marking this as const and while LLDB uses it a bit more this isn't common at all. The current code now makes you wonder why this is different than has_extra_args. Even if that does get modified in some of the function's implementation, that's an implementation detail that shouldn't be exposed through the interface.