[cfe-commits] [PATCH] Diagnose cv-qualified operator new/delete

[Jordan Rose]

On Jul 30, 2012, at 3:03 PM, Aaron Ballman <aaron at aaronballman.com> wrote:

> On Mon, Jul 30, 2012 at 4:41 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> On Mon, Jul 30, 2012 at 4:37 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>> 
>>> On Jul 30, 2012, at 13:28 , Aaron Ballman <aaron at aaronballman.com> wrote:
>>> 
>>>> This patch addresses PR13481 so that we properly diagnose overloaded
>>>> operator new and operator delete if it is cv-qualified.
>>> 
>>> How well does this recover? I think we should just fixit-remove the 'const' or 'volatile' and continue as if the declaration is correct, but it looks like we're actually going to skip the decl pretty much entirely.
>> 
>> The fixit is a good idea, I'll incorporate it.
> 
> There may be a problem (or an educational issue).  AFAICT, there's no
> way to find the source range for the cv-qualifiers, so I cannot create
> a removal fixit for the declaration.

We should probably fix this someday; if you don't plan to do it now, please file a PR.


> What's more, I think I'd also
> have to create a removal for the definition as well (or else we'll
> simply get a different compile error).  Ideas are welcome, but for
> right now I've left it as a FIXME.

This I don't understand. Each time we see a declaration, we can remove any qualifiers, no? As long as we do this before we check redeclarations there shouldn't be a problem AFAICT. Though we'd have to be careful about templates, which may cause the warning to be emitted more than once.

I think even without actually emitting a textual fixit, we should still remove the qualifier from the FunctionType and continue, so that we get sane compilation of the rest of the program.


> 
>>> Also, the internal warning name has "newdelete" in it, but the text does not. Perhaps this should be used for all static methods, and possibly constructors and destructors as well?
>> 
>> Not a bad idea, it is a general error.  I'll refactor into something
>> more generally reusable.
> 
> I've moved the functionality into its own Sema function, and renamed
> the diagnostic to be more agnostic.  Once this patch lands, I can look
> into adding/refactoring other functionality.

Sounds good. Some notes on the patch itself: you have a missing close quote when citing the standard, and your patch has a number of whitespace-only changes that should be excluded (even though they're all removing trailing whitespace).

Would be nice if someone a little more familiar with SemaCXX could comment here as well.
Jordan