[Lldb-commits] [PATCH] D24013: Removed the `GetStringRef()` functions of `StringExtractor`

[Pavel Labath via lldb-commits]

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

It is hung because one of your previous changes broke a very fundamental piece of functionality, which makes all tests fail. It is fixed now http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake and the tests are passing on our buildbot in a very stable manner.

I request that you stop making these changes, until we figure out how to make sure they can go in safely.

- first, I'd like to wait with the changes until after the reformat. The reason for this is that the reformat will make tracking breakages and/or reverting much more difficult. I'm not disputing the need for doing this, but I do believe it can wait one more week.
- second, I'd like to have these changes tested more before or immediately after they go in. The StringExtractor is much more heavily used on OSX/Linux than it is on windows, so I don't think running lldb windows host tests is enough. Now, there is nothing host-specific in the string extractor and in an ideal world we would have enough unit tests to make sure a refactor is not breaking things, but we are not there yet, and until we get there, I think you'll need to be more careful about refactoring random stuff.

You said the test suite was not working for you yesterday. Could you check it again today? It works fine on the buildbot now. Last time I remember we spoke about this, the problem  was two tests being too slow. Both of these issues should be resolved now, and if there is still something wrong, I am ready to work with you to resolve them. This should enable you to have more confidence into the more risky changes. For the less risky ones, you should be able to rely on the buildbot to do post-submit verification.  I'll get the buildbot set up to send out emails when tests break. I think it is very stable now, and I think I've been putting of that task for long enough. Please look out for emails from it and try to resolve problems it detects, particularly when making chained changes like yesterday. The turnaround time should be very fast now (12 minutes). I'd like to avoid situations where I have to scramble to put things back in a working state, like I had to for the past two mornings.

Again, I am not trying do stop you from doing this, as I agree that it needs to be done. I'd like to just make sure it is done in a sustainable manner.


https://reviews.llvm.org/D24013