ref qualify parser methods to avoid use of dangling objects #703
Conversation
|
1 |
|
It sounds like a great thing! I presume this means it'll also work on pointers with parser->parse()? |
You are absolutely right, the failure tests should be individual examples. The lazy way out definitely could miss something! |
Honestly, I'd hide them in doxygen and document the limitation in the normal variant--having |
I think it's fine! I've been super worried about the temporary issue, and didn't realize there was a way to do this. This makes me even more comfortable than before. |
Glad you liked it! Yes, parser->parse() works fine with this change. |
To avoid using data belonging to a temporary, the parse functions are ref qualified to get a compile error if used on an rvalue. See #696
I tried to understand the existing fail to compile tests, but I am not sure how the example without exceptions is intended to work, is it supposed to be a mirror of the working example code ? I am worried that having too much code in a fail to compile code means it is likely to fail due to for instance api changes, and one falsely believes everything is fine.
So I stole the concept and (hopefully:-) improved on it by keeping each file small, with as little code as possible changing between the "supposed to compile" code and "supposed to fail" code. See this pull request as the start of a discussion! Could you @jkeiser please have a look?
Perhaps the deleted overloads should have doxygen comments or a regular comment so people seeing reference qualifications for the first time are not surprised?
I had to update the benchmark which correctly used a temporary, this is a consequence of using this technique to capture the #696 type of mistakes. This means this is potentially a breaking change, but it results in a failed compile so it will be easy to fix.
In case the technique in this pull request is accepted, perhaps the release notes should say something about it so people are not surprised when it stops compiling.