Clean up ConstraintVariable copy constructors/getCopy function

[kyleheadley]

ConstraintVariable(CV) and subclasses have copy constructors and a function getCopy. Neither is documented and the seem to be similar. It looks like some authors even thought they were. But getCopy creates constraints, and seems to be used for function return r-values. Copy constructors may only be in support of this function, but we shouldn't be creating new CVs if we can appropriately build constraints off of them.

Rename and document "getCopy" to reflect its use. Determine when actual copies are appropriate and clean up or even delete the copy constructors.

[john-h-kastner]

Regarding the difference, here is the documentation currently in the code:

checkedc-clang/clang/include/clang/3C/ConstraintVariables.h

Lines 218 to 221 in 50e0a25

	// Copy this variable and replace all VarAtoms with fresh VarAtoms. Using
	// fresh atoms allows the new variable to solve to different types than the
	// original.
	virtual ConstraintVariable *getCopy(Constraints &CS) = 0;

checkedc-clang/clang/include/clang/3C/ConstraintVariables.h

Lines 367 to 371 in 50e0a25

	// Construct a copy of this variable, reusing all VarAtoms. To instead obtains
	// a copy of the constraint variable which contains fresh VarAtoms (allowing
	// the new variable to solve to different type than the original), instead use
	// the method getCopy.
	PointerVariableConstraint(PointerVariableConstraint *Ot);

so, the difference is that getCopy allows the copy to solve to a different type, which is needed when constructing the constraint variables for function returns. Both allocate a new constraint variables, which afaik is necessary everywhere they're used . Naming can, of course, be improved.

The copy constructor is used in getCopy, but also when constructing constraints for address-of and pointer dereference operators in derefPVConstraint and addAtomPVConstraint. If there is an oportunity for substantial simplification, these could possibly be reimplemented to create a copy of the constraint with fresh VarAtoms (as is done by getCopy) and then create appropriate constraints between the original and the copy.

[kyleheadley]

I guess it's not as bad as the first impression I got. Especially as some of the getCopy atoms were constrained against their copies' atoms, but not all of them. And it would be nice to have a higher-level explanation, that is, why would you want to use one over the other, or at all? Maybe getCopy is more convenient in some cases than getting a fresh var, because some of the data is filled in? And if the copy constructor is only used in deref and similar cases where the same pointers exist than that makes sense. Maybe we can make the "copy" part into private functions and only expose the functions that make proper use of it?

[john-h-kastner]

Maybe we can make the "copy" part into private functions and only expose the functions that make proper use of it?

Good point. getCopy shouldn't be public. I think it would make sense to encapsulate this whole block (and maybe clean it up a bit, I feel like the PCV->isOriginallyChecked() branch shouldn't be necessary).

checkedc-clang/clang/lib/3C/ConstraintResolver.cpp

Lines 524 to 545 in 50e0a25

	ConstraintVariable *NewCV;
	auto *PCV = dyn_cast<PVConstraint>(CV);
	if (!IsAllocator) {
	if (PCV && PCV->isOriginallyChecked()) {
	// Copying needs to be done differently if the constraint variable
	// had a checked type in the input program because the constraint
	// variables contain constant atoms that are reused by the copy
	// constructor.
	auto *NewPCV =
	new PVConstraint(CE->getType(), nullptr, PCV->getName(), Info,
	*Context, nullptr, PCV->getGenericIndex());
	NewCV = NewPCV;
	if (PCV->hasBoundsKey())
	NewCV->setBoundsKey(PCV->getBoundsKey());
	} else {
	NewCV = CV->getCopy(CS);
	}
	} else {
	// Allocator functions are treated specially, so they do not have
	// separate parameter and argument return variables.
	NewCV = CV;
	}