- User Since
- Nov 12 2014, 1:58 PM (213 w, 5 d)
commit 44b5014ce138fa293238afeaa53cfd4c2f5b12d2 (HEAD -> master)
Author: Davide Italiano <email@example.com>
Date: Mon Dec 17 10:23:44 2018 -0800
Sun, Dec 16
Fri, Dec 14
Thu, Dec 13
OK, I'll try to review it later today. Will you have time to commit this or you want any of us to do it on your behalf?
So, is there anything remaining before we can merge this patch?
Wed, Dec 12
Mon, Dec 10
Thu, Dec 6
Wed, Dec 5
I'll switch this to be a DenseMap.
Tue, Dec 4
Updated to address Adrian's comments.
patch with context.
Mon, Dec 3
No objections from me, but I would appreciate if @labath can take a look.
LGTM, sorry. for the delay.
Sat, Dec 1
LGTM conditional to lack of objections from people on llvm-dev.
Wed, Nov 28
I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too).
To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on.
I'm worried about the broader trend of landing patches to ASTImporter without proper reviews and testing as I am of this very instance.
I really think you should consider asking reviews to somebody who's familiar with clang and test lldb as it's the main client of this functionality we have in tree.
This doesn't break anything, and I'm fairly confident Raphael (@teemperor) ran the tests.
As somebody who used bugpoint fairly extensively over the course of the years, I have to say I always considered a little error prone the fact that bugpoint looks up in the PATH to find opt.
So, I tend to be in favor of this patch, but I'm wondering whether it's worth to look in the $PATH instead of just looking in the current directory.
Realistically bugpoint is a tool for developers, so chances are you're going to run it directly from bin using the compiler/interpreter/assembler you just built.
Tue, Nov 27
Mon, Nov 26
Mon, Nov 19
LGTM. Given this is under EXPENSIVE_CHECK, we might consider a more thorough verification?
Sun, Nov 18
I don't have any other great ideas either and I'd say the proposed alternative is not worth the complexity.
Alexsei, I'm afraid I'm not qualified to review this patch. I would really recommend you to find somebody who's familiar with clang to review it, as it already seems to have broken lldb in the past.
Is there a way of fixing this that doesn't require scattering the test between two files?
Nov 16 2018
Nov 15 2018
Nov 13 2018
Nov 12 2018
This is trivial. I don't know why we didn't do this before.
Thanks for addressing my comments.
Nov 8 2018
Nov 7 2018
Nov 6 2018
@lanza Hey Nathan, it looks like your test exposed an UB in ELFObjectFileReader. May I ask you to take a look?
Nov 5 2018
LGTM. Thanks for picking up the slack.
Nov 3 2018
Nov 2 2018
This broke MacOS. I'm going to revert this. To reproduce, just run ninja check-lldb with your patches.
Please let me know if you need other informations.
Nov 1 2018
Oct 31 2018
I'll take a look early next week. Sorry, I don't work on scalar optimizations anymore on llvm these days, so I need to page this code in again.
No worries :) l
Oct 30 2018
This patch broke lldb, at least on MacOS.
You can reproduce the issue building lldb, applying your patch and then running