search

JossWhittle / FlintPlusPlus

Flint++ is cross-platform, zero-dependency port of flint, a lint program for C++ developed and used at Facebook.
Boost Software License 1.0
266 stars 21 forks source link

Using string iterator instead of creating new strings #15

Closed kanielc closed 10 years ago

kanielc commented 10 years ago

So instead of chomping the string we're processing, instead use an iterator into the data (since it's always a substring of input).

This avoids a bunch of string re-allocations. The best part is that the performance is now much better.

Run time from the original test run (a few commits ago): Flint++ - 0m0.987s Present run time after the iterator and file changes: real 0m0.281s It's actually faster now than Facebook's C++ version :)

By the way, this is the profile of the project tested on: Lint Summary: 642 files Errors: 91 Warnings: 397 Advice: 268

@L2Program I had to comment out some assert calls because I no longer had access to .size() from the iterator. I'll leave it up to you to decide what we should do with them.

JossWhittle commented 10 years ago

I get an iterator out of range error when I run Flint++ on itself. Can you try to replicate this? I'll have a debug now and see if I can find the problem

JossWhittle commented 10 years ago

Specifically the line of stl code which generates the error shows this as the culprit.

_DEBUG_ERROR("string iterator not dereferencable");
kanielc commented 10 years ago

Just ran it locally (both the branch and master), didn't get any errors.

What compiler do you have? I'm on GCC 4.8.2

JossWhittle commented 10 years ago

MSVC++ '13

JossWhittle commented 10 years ago

I believe it's coming from the use of &*pc

kanielc commented 10 years ago

Interesting... I think it's a compiler thing, because that should be valid. I'll look into it more (maybe later today as I have an appointment).

I'm getting to the char in the iterator and then just getting a direct pointer to it.

kanielc commented 10 years ago

OK, I added a bit of a safety check elsewhere in a new PR, but I'll try at this one too later.