search

tkruse / cpplint

Fork of google cpplint, deprecated
3 stars 4 forks source link

Fix rvalueref whitespace #24

Closed tkruse closed 8 years ago

tkruse commented 8 years ago

@itamaro if you have time, please review this too. The unit test fail without the fix, I think they may demonstrate the 2 bugs. Mostly you should check wether this introduces false positives, or whether you see any other chars missing in those regeses to fix more similar issues.

Honestly, after digging through this code to find the bug(?), I understand why Google does not maintain cpplint anymore.

itamaro commented 8 years ago

oh my, this is way too complex to debug... pretty amazing that you were able to pull it off!

this is exactly the kind of regex-based parsing that demonstrates why it's a bad idea to try to parse a programming language this way... I don't know what Google does internally these days, but I wouldn't be surprised if they switched to assertions on AST's taken from LLVM or something like that.

anyway, I stared at the diff for a while, until my vision blurred. I'm 100% certain that it misses things and has false positives, but I can also say that on the previous version, and on any regex-based version for that matter :-) but I don't have any specific examples or suggestions, so I'd say go ahead with it, since you provide decent tests.

if you have a decent real-world codebase, that could be good enough to test for "real world false positives" - run with and without this change and compare the outputs. any diverse C++ codebase would do I guess - like this one

tkruse commented 8 years ago

Well, cpplint is not correct or complete, does not even try to, it uses many assumptions to be right 99% of the times in a Google-controlled codebase. I only realized this after reading the code. This path in particular is already know not to cover a situation lile

void create_output(const char *c,
                   A&& a);

and I am not sure if I should try to fix that one as well (having to expand the regex search over at least the last line.

Anyway, I can merge this for now, and keep track that bug as a separate issue