Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Patch LGTM, please wait for Rui approval.
| ELF/Target.h | ||
|---|---|---|
| 150 ↗ | (On Diff #126098) | nit: I would shorten this: |
| ELF/Target.h | ||
|---|---|---|
| 150 ↗ | (On Diff #126098) | Sounds good, I'll change if it to that if Rui also agrees. |
| ELF/Target.h | ||
|---|---|---|
| 171 ↗ | (On Diff #126098) | Since we only call this with values <= INT64_MAX that is much better, thanks. |
| ELF/Target.h | ||
|---|---|---|
| 171 ↗ | (On Diff #126098) | But why INT64_MAX? It seems just inconsistent. If you sign-extend, you want to always do that, no? |
| ELF/Target.h | ||
|---|---|---|
| 171 ↗ | (On Diff #126098) | I meant that this function is either called with a negative signed number or an unsigned value that doesn't have the int64 sign bit set so that using Twine((int64_t)V) is exactly the same as the ternary expression that I had here. |
| ELF/Target.h | ||
|---|---|---|
| 171 ↗ | (On Diff #126098) | If Twine((int64_t)V) is exactly the same as the ternary expression, please use the simpler form which is Twine((int64_t)V). |
| ELF/Target.h | ||
|---|---|---|
| 171 ↗ | (On Diff #126098) | Yes that is exactly what I meant. Sorry about the miscommunication. |
LGTM
| test/ELF/i386-reloc-16.s | ||
|---|---|---|
| 12 ↗ | (On Diff #126393) | Please remove an extra space after "," (you have two spaces after ","). |
| test/ELF/i386-reloc-8.s | ||
| 12 ↗ | (On Diff #126393) | Ditto |
| test/ELF/x86-64-reloc-16.s | ||
| 12 ↗ | (On Diff #126393) | Ditto |
| test/ELF/x86-64-reloc-8.s | ||
| 12 ↗ | (On Diff #126393) | Ditto |
| test/ELF/x86-64-reloc-error.s | ||
| 9 ↗ | (On Diff #126393) | Ditto |