This is an archive of the discontinued LLVM Phabricator instance.

Removed the `GetStringRef()` functions of `StringExtractor`
Needs RevisionPublic

Authored by zturner on Aug 29 2016, 4:17 PM.

Details

Reviewers
tberghammer
Summary

The motivation here was that operations which were using the const version of this function might as well be using Peek(), since it does essentially the same thing. And since Peek() was updated to return StringRef, it even provides a more useful interface than std::string.

On the other hand, operations which were using the non const version of the function won't work in an environment where StringExtractor stores a StringRef instead of a std::string, and on top of that they are unsafe since modifying the StringExtractor by exposing its internals breaks encapsulation and requires the user to remember to update the file pos lest the class enter an invalid state.

So I add a new SetString() method. Instead of calling GetStringRef() and then manipulating the result, you just manipulate a std::string you create yourself, and then call SetString().

Because calls to the const version of GetStringRef() are changed to using Peek(), certain functions that were reproducing functionality already built in to StringRef have been simplified quite a bit.

Diff Detail

Event Timeline

zturner updated this revision to Diff 69631.Aug 29 2016, 4:17 PM
zturner retitled this revision from to Removed the `GetStringRef()` functions of `StringExtractor`.
zturner updated this object.
zturner added reviewers: labath, tberghammer.
zturner added a subscriber: lldb-commits.
zturner added inline comments.Aug 29 2016, 4:29 PM
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
1677

Also, was this a bug? I fixed it because it certainly looks like a bug, but if this is intended behavior please let me know.

zturner updated this revision to Diff 69635.Aug 29 2016, 4:31 PM

Actually fix the bug this time instead of making worse.

labath edited edge metadata.Aug 30 2016, 6:13 AM

Which revision is this patch against? It does not seem to apply cleanly to trunk for me. Could you try running the test suite on linux or mac as well, as the last StringRef change left things in a pretty broken state there (you are changing code which is pretty-much unused on windows)? If not I can give it a shot once it the merge issues are resolved.

Apart from that, I like the direction this is going in.

unittests/Utility/StringExtractorTest.cpp
76

This seems unnecessary.

zturner updated this revision to Diff 69741.Aug 30 2016, 1:00 PM
zturner edited edge metadata.

Rebased after submitting all local changes. This should apply cleanly on top of LLDB r280139.

zturner updated this revision to Diff 69750.Aug 30 2016, 1:45 PM

Fixed a few build errors on Linux. Test suite is running right now, but it's taking a long time so it may be hung (on the other hand, I actually don't remember the last time I had a successful test run on Linux, even without any of my patches).

labath requested changes to this revision.Aug 31 2016, 6:26 AM
labath edited edge metadata.

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.

This revision now requires changes to proceed.Aug 31 2016, 6:26 AM
labath added a comment.Sep 8 2016, 4:28 AM

The reformat is complete, and our main buildbot is (as I think you've already found out) in the non-experimental mode. As far as I am concerned, we can proceed with this. If you can't test on linux before hand, just look out for the emails and be ready to respond. If there's anything I can do to make this process easier (figure out why the linux test suite is not working on your side?), let me know.

labath resigned from this revision.Apr 20 2017, 6:15 AM