Hi!
I broke couple of times MSVC builds when committed minor code
cleanups. Looks like problem occurred because of changed order of
headers.
I could not investigated problems myself, because I don't have access to MSVC.
Changes in question:
r250872:
source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h
r250925:
source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
source/Plugins/OperatingSystem/Go/OperatingSystemGo.h
Eugene.
I’m taking a look now. In the future please revert changes that break a buildbot. If you think you might know what the fix is (i.e. you guess that including a header will fix it), then you can just check it in as a test. If after a few attempts you still can’t figure it out, then revert the change out again.
But definitely please don’t leave broken changes in. For example, the MSVC buildbot is now failing due to changes in LLVM. The reason is because it broke last night due to some unrelated changes which weren’t reverted, and subsequent breaks did not result in an email to other people so they didn’t know. We need to avoid this situation, so the bot has to stay green.
anyway I’m working on fixing it now
Hi, Zachary!
I'm taking a look now. In the future please revert changes that break a
buildbot. If you think you might know what the fix is (i.e. you guess that
including a header will fix it), then you can just check it in as a test.
If after a few attempts you still can't figure it out, then revert the
change out again.But definitely please don't leave broken changes in. For example, the MSVC
buildbot is now failing due to changes in LLVM. The reason is because it
broke last night due to some unrelated changes which weren't reverted, and
subsequent breaks did not result in an email to other people so they didn't
know. We need to avoid this situation, so the bot has to stay green.anyway I'm working on fixing it now
At first I want to apologize for broken builds.
In two previous cases were reason was more or less clear, I reverted
my changes after receiving buildbot notifications.
I was not sure about last case, since fix should be made in file which
I didn't change in previous commit.
Sure, I'll try to bu more careful in future.
Eugene.
It looks like you’re generating these diffs from an SVN repo, so I’m seeing lldb/trunk in the paths. Because of that, I can’t apply it on my git repo which doesn’t have the “trunk” folder. Can you generate the diffs for the 2 aforementioned CLs by cding into the trunk directory and generating the diff there?
For the disassembler patch, the problem is you’ve defaulted the destructor but you’ve got a unique_ptr to a forward declared class. MSVC is actually correct in failing to compile this, and I’m not sure why other compilers are accepting it. My guess is something to do with the order of includes from the cpp file.
To fix this you need to remove the default keyword from the destructor and provide an empty implementation of the destructor in DisassemblerLLVMC.cpp
This problem occurs in other places too. In OperatingSystemGo.h the same problem exists with DynamicRegisterInfo. That’s the one causing the operating system patch to fail.
What compiler are you testing this with? This should fail under clang as well, I’m surprised it doesn’t. Are you using GCC by chance?
In any case, you’ll need to fix all the occurrences of having a defaulted destructor in a class with a std::unique_ptr of an incomplete type.
I think including missing file will be better solution.
I use trunk (~ week old) Clang.
Eugene.
The classes appear to be all forward declared intentionally, so you’d need to include a lot of missing files. Just moving the destructor to cpp file should be pretty easy and still allow to keep the header footprint down, which speeds up build times.