- User Since
- Oct 10 2016, 10:44 AM (127 w, 3 d)
@davide : is that ok if I add you to a few other python compat related reviews?
validates fine on my side with py3
I didn't run the check-lldb target though, I'm doing that right now.
@davide I first applied 2to3 systematically, then manually edited the diff so that it remains backward compatible with python2.
Thanks for the review! I've started to work on lldb python code base, and with that we should have all Python files for llvm, clang, lld and lldb compatible with Python2 and Python3.
Tue, Mar 19
Fri, Mar 15
Wed, Mar 6
I confirm the bug has been fixed in trunk gcc and in fedora rawhide's gcc, closing the issue.
I'd rather have it here than in lit. As a test environment, I love it when there's as few environment values that depends on the launch site as possible. It makes reproducibility easier.
LGTM, do you have commit rights?
Thu, Feb 28
Tue, Feb 26
@jmittert sorry for the long delay, but I'm finally fine with this patch now. I like how it explicitly emphasizes on the Windows/Linux difference. However the patch needs to be rebased against master, can you update it?
I've reported this strange behavior to gcc https://bugzilla.redhat.com/show_bug.cgi?id=1678613
@sylvestre.ledru up :-)
Mon, Feb 25
@sgraenitz It's fixed! I'm dropping this patch then, thanks for investigating o/
Wed, Feb 20
@sgraenitz : it's possible that r352382 fixes my issue. I'll double check that first!
@hans agreed; Thanks for taking the time to try to reproduce the original issue o/
Feb 19 2019
Fedora is currently using gcc-9.0.1, svn revision 268719 (see https://src.fedoraproject.org/rpms/gcc/blob/master/f/gcc.spec) which triggers this issue. I'll forward the test case to the gcc team and report here.
Feb 18 2019
Some follow-up on this review: I had to revert the associated commit because it was breaking on buildbot, on test case I failed to reproduce locally.
@sgraenitz I currently have this patch applied to LLVM 8rc1 source tree for fedora, because it wasn't working automagically otherwise. Reading the discussion, I don't think I missed some configuration stuff, or what did I miss when configuring the build of lldb in standalone mode?
Feb 17 2019
Yeah, especially as he copy constructor is deleted, push_back implies a copy, so relying on the compiler to optimize to a move seems fragile.
Feb 16 2019
Feb 15 2019
Feb 14 2019
Feb 13 2019
Feb 12 2019
Feb 11 2019
@MarcoFalke it's in, thanks for the patch o/
LGTM, do you have commit right?
Feb 9 2019
@chandlerc patch updated, using inheritance to avoid too much duplication.
Feb 8 2019
@jmittert I tried the following (simpler) patch on Linux and it seems to work nice for both Python2 and Python3
@chandlerc there's indeed some redundancy that can be removed unfortunately, as you feared, defaulting seems incompatible with per-member specialization, or at least it hits my c++ knowledge limit.
Feb 5 2019
@andrew-boyarshin : It's in! Thanks for the contribution o/
@andrew-boyarshin do you have commit right?
Jan 31 2019
It's in, thanks @rsmith for the review.
Jan 30 2019
Patch updated to take into account @rsmith remarks. Test case added as an illustration.
Jan 28 2019
Jan 24 2019
Also validates ok with clang 6.0.1 and gcc 4.8.5
@ruiu It works fine, I just moved the comdat-related insertion after the flag check. It compiles and validates fine on my setup.
Thanks @roxma for the patch!
Jan 23 2019
This validates with gcc 8.2. Given the history of llvm::Optional, I'm going to try to build with other compilers too.
@ruiu is this minimal patch ok to you?