This is an archive of the discontinued LLVM Phabricator instance.

Make Twine's copy constructor private
ClosedPublic

Authored by zturner on Oct 10 2017, 3:04 PM.

Details

Summary

I was running across a lot of abuse of Twine, some of which probably worked, and some of which came very close to skirting the line of safety. Examples of this include:

  1. Accepting Twine by value instead of by const reference.
  2. Returning Twines by value from functions.
  3. Storing Twines by value in classes.

and others. The 3rd of those mentioned was particularly onerous because it can happen without you even knowing it, for example if you have:

template<class T>
struct storage {
  T t;
};

and then you use a function such as make_storage to perfectly forward args through, and one of those happens to be a twine.

Ultimately, Twine really should not be copyable. The header file even hints at this by saying "functions should only accept Twine by const reference". But since this rule is not enforced, many violations of this were occurring.

I've made the copy constructor private here (since Twine internally has a few valid use cases for a copy constructor), and fixed up all uses.

In testing this, I noticed that there were no unit tests for Twine's operator helpers, so I added some there (which incidentally exposed a bug in the implementation of printOneRepr, which is also now fixed)

Diff Detail

Event Timeline

zturner created this revision.Oct 10 2017, 3:04 PM
dblaikie accepted this revision.Oct 10 2017, 3:14 PM
dblaikie added inline comments.
llvm/include/llvm/ADT/Twine.h
176

I'd probably skip the "Copy constructor is private" - as I'm guessing that's fairly obvious from context.

& "We need" - I think it's usually better to impersonalize comments "The copy constructor is used internally but Twines are not intended to be publicly copyable." perhaps.

This revision is now accepted and ready to land.Oct 10 2017, 3:14 PM
This revision was automatically updated to reflect the committed changes.