@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.

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?

@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 :-)

@sgraenitz It's fixed! I'm dropping this patch then, thanks for investigating o/

@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/

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.

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.

In D58117#1401457, @dblaikie wrote:

There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )

@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?

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.

@MarcoFalke it's in, thanks for the patch o/

LGTM, do you have commit right?

@chandlerc patch updated, using inheritance to avoid too much duplication.

@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.

up!

@andrew-boyarshin : It's in! Thanks for the contribution o/

@andrew-boyarshin do you have commit right?

It's in, thanks @rsmith for the review.

Patch updated to take into account @rsmith remarks. Test case added as an illustration.

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!

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?